xdp: separate struct xdp_prog as container for bpf_prog
Subsystems (7)
RFC PATCH 1/1 xdp: separate struct xdp_prog as container for bpf_prog
Currently the XDP program is simply a bpf_prog pointer. While it is good for simplicity, it is limiting extendability for upcoming features.
Introducing a new struct xdp_prog, that can carry information related to the XDP program. Notice this approach does not affect performance (tested and benchmarked), because the extra dereference for the eBPF program only happens once per 64 packets in the poll function.
The features that need this is:
* Multi-port TX:
Need to know own port index and port lookup table.
* XDP program per RX queue:
Need setup info about program type, global or specific, due to replace semantics.
* Capabilities negotiation:
Need to store information about features program want to use, in-order to validate this.
I do realize this new struct xdp_prog features cannot go into the kernel before one of the three users of the struct is also implemented.
--- drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 12 +++--- drivers/net/ethernet/mellanox/mlx4/en_rx.c | 10 +++-- drivers/net/ethernet/mellanox/mlx4/mlx4_en.h | 2 - include/linux/filter.h | 14 +++++++ include/linux/netdevice.h | 2 - net/core/dev.c | 15 +++++-- net/core/filter.c | 50 ++++++++++++++++++++++++ 7 files changed, 89 insertions(+), 16 deletions(-)
drivers/net/ethernet/mellanox/mlx4/en_netdev.c +6 -6
@@ -2622,11 +2622,11 @@ static int mlx4_en_set_tx_maxrate(struct net_device *dev, int queue_index, u32 mreturn err; } -static int mlx4_xdp_set (struct net_device *dev,struct bpf_prog *prog) +static int mlx4_xdp_set (struct net_device *dev,struct xdp_prog *prog) {struct mlx4_en_priv *priv =netdev_priv (dev);struct mlx4_en_dev *mdev = priv->mdev; -struct bpf_prog *old_prog; +struct xdp_prog *old_prog;int xdp_ring_num;int port_up =0 ;int err;
@@ -2639,7 +2639,7 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog)*/ if (priv->xdp_ring_num == xdp_ring_num) {if (prog) { - prog =bpf_prog_add (prog, priv->rx_ring_num -1 ); + prog =xdp_prog_add (prog, priv->rx_ring_num);if (IS_ERR (prog))return PTR_ERR (prog); }
@@ -2650,7 +2650,7 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog) lockdep_is_held(&mdev->state_lock));rcu_assign_pointer (priv->rx_ring[i]->xdp_prog, prog);if (old_prog) -bpf_prog_put (old_prog); +xdp_prog_put (old_prog); }mutex_unlock (&mdev->state_lock);return 0 ;
@@ -2669,7 +2669,7 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog) }if (prog) { - prog =bpf_prog_add (prog, priv->rx_ring_num -1 ); + prog =xdp_prog_add (prog, priv->rx_ring_num);if (IS_ERR (prog))return PTR_ERR (prog); }
@@ -2690,7 +2690,7 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog) lockdep_is_held(&mdev->state_lock));rcu_assign_pointer (priv->rx_ring[i]->xdp_prog, prog);if (old_prog) -bpf_prog_put (old_prog); +xdp_prog_put (old_prog); }if (port_up) {
drivers/net/ethernet/mellanox/mlx4/en_rx.c +6 -4
@@ -535,13 +535,13 @@ void mlx4_en_destroy_rx_ring(struct mlx4_en_priv *priv, {struct mlx4_en_dev *mdev = priv->mdev;struct mlx4_en_rx_ring *ring = *pring; -struct bpf_prog *old_prog; +struct xdp_prog *old_prog; old_prog =rcu_dereference_protected ( ring->xdp_prog,lockdep_is_held (&mdev->state_lock));if (old_prog) -bpf_prog_put (old_prog); +xdp_prog_put (old_prog);mlx4_free_hwq_res (mdev->dev, &ring->wqres, size * stride +TXBB_SIZE );vfree (ring->rx_info); ring->rx_info =NULL ;
@@ -783,7 +783,8 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int budstruct mlx4_en_rx_ring *ring = priv->rx_ring[cq->ring];struct mlx4_en_rx_alloc *frags;struct mlx4_en_rx_desc *rx_desc; -struct bpf_prog *xdp_prog; +struct xdp_prog *xdp_prog; +struct bpf_prog *bpf_prog;int doorbell_pending;struct sk_buff *skb;int tx_index;
@@ -805,6 +806,7 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud/* Protect accesses to: ring->xdp_prog, priv->mac_hash list */ rcu_read_lock (); xdp_prog =rcu_dereference (ring->xdp_prog); + bpf_prog =rcu_dereference (xdp_prog->bpf); doorbell_pending =0 ; tx_index = (priv->tx_ring_num - priv->xdp_ring_num) + cq->ring;
@@ -897,7 +899,7 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud frags[0 ].page_offset; xdp.data_end = xdp.data + length; - act =bpf_prog_run_xdp (xdp_prog, &xdp); + act =bpf_prog_run_xdp (bpf_prog, &xdp);switch (act) {case XDP_PASS :break ;
drivers/net/ethernet/mellanox/mlx4/mlx4_en.h +1 -1
@@ -340,7 +340,7 @@ struct mlx4_en_rx_ring {u8 fcs_del;void *buf;void *rx_info; -struct bpf_prog __rcu *xdp_prog; +struct xdp_prog __rcu *xdp_prog;struct mlx4_en_page_cache page_cache;unsigned long bytes;unsigned long packets;
include/linux/filter.h +14 -0
@@ -437,6 +437,20 @@ struct xdp_buff {void *data_end; }; +struct xdp_prog { +u64 flags; +struct bpf_prog *bpf; +atomic_t refcnt; +struct rcu_head rcu;// Do we need RCU freeing? likely right? + +/* Data associated with XDP program goes here */ + +}____cacheline_aligned_in_smp ; + +struct xdp_prog *xdp_prog_alloc (struct bpf_prog *bpf); +struct xdp_prog *xdp_prog_add (struct xdp_prog *xdp,int users); +void xdp_prog_put (struct xdp_prog *prog); +/* compute the linear packet data range [data, data_end) which * will be accessed by cls_bpf and act_bpf programs */
include/linux/netdevice.h +1 -1
@@ -825,7 +825,7 @@ struct netdev_xdp {enum xdp_netdev_command command;union {/* XDP_SETUP_PROG */ -struct bpf_prog *prog; +struct xdp_prog *prog;/* XDP_QUERY_PROG */ bool prog_attached; };
net/core/dev.c +11 -4
@@ -6643,23 +6643,30 @@ EXPORT_SYMBOL(dev_change_proto_down);int dev_change_xdp_fd (struct net_device *dev,int fd) {const struct net_device_ops *ops = dev->netdev_ops; -struct bpf_prog *prog =NULL ; +struct xdp_prog *prog =NULL ; +struct bpf_prog *bpf;struct netdev_xdp xdp = {};int err;if (!ops->ndo_xdp)return -EOPNOTSUPP ;if (fd >=0 ) { - prog =bpf_prog_get_type (fd,BPF_PROG_TYPE_XDP ); -if (IS_ERR (prog)) + bpf =bpf_prog_get_type (fd,BPF_PROG_TYPE_XDP );/* inc refcnt */ +if (IS_ERR (bpf)) +return PTR_ERR (bpf); + + prog =xdp_prog_alloc (bpf); +if (IS_ERR (prog)) { +bpf_prog_put (prog->bpf);return PTR_ERR (prog); + } } xdp.command =XDP_SETUP_PROG ; xdp.prog = prog; err = ops->ndo_xdp (dev, &xdp);if (err <0 && prog) -bpf_prog_put (prog); +bpf_prog_put (prog->bpf);return err; }
net/core/filter.c +50 -0
53 lines not on this page · raw message
Daniel Borkmann
maintainerQuoted 19 lines · Jesper Dangaard Brouer
Currently the XDP program is simply a
bpf_progpointer. While it is good for simplicity, it is limiting extendability for upcoming features.Introducing a new struct
xdp_prog, that can carry information related to the XDP program. Notice this approach does not affect performance (tested and benchmarked), because the extra dereference for the eBPF program only happens once per 64 packets in the poll function.The features that need this is:
* Multi-port TX:
Need to know own port index and port lookup table.* XDP program per RX queue:
Need setup info about program type, global or specific, due to replace semantics.
Quoted 6 lines
* Capabilities negotiation:
Need to store information about features program want to use, in-order to validate this.I do realize this new struct
xdp_progfeatures cannot go into the kernel before one of the three users of the struct is also implemented.
Yep, so this whole wrapper structure which adds yet another mgmt layer with ref'counting etc is kind of hard to judge whether it's into the right direction w/o showing a user of this struct. Each BPF prog does have struct bpf_prog_aux, where we store auxiliary data, so I'd recommend to try extending this with some private data, which can be for XDP type etc since we know it's an XDP prog anyway. Or is there a specific reason that this is impossible to do, resp. limiting?
Quoted 4 lines · Jesper Dangaard Brouer
Currently the XDP program is simply a
bpf_progpointer. While it is good for simplicity, it is limiting extendability for upcoming features.
Hi Jesper,
Can you take a look (or try) the RFC patches I just posted to generalize XDP. I believe that should subsume most of what you're doing here!
Thanks,
Tom
Quoted 283 lines
Introducing a new struct
xdp_prog, that can carry information related to the XDP program. Notice this approach does not affect performance (tested and benchmarked), because the extra dereference for the eBPF program only happens once per 64 packets in the poll function.The features that need this is:
* Multi-port TX:
Need to know own port index and port lookup table.* XDP program per RX queue:
Need setup info about program type, global or specific, due to replace semantics.* Capabilities negotiation:
Need to store information about features program want to use, in-order to validate this.I do realize this new struct
xdp_progfeatures cannot go into the kernel before one of the three users of the struct is also implemented.Signed-off-by Jesper Dangaard Brouer <[email protected]>--- drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 12 +++--- drivers/net/ethernet/mellanox/mlx4/en_rx.c | 10 +++-- drivers/net/ethernet/mellanox/mlx4/mlx4_en.h | 2 - include/linux/filter.h | 14 +++++++ include/linux/netdevice.h | 2 - net/core/dev.c | 15 +++++-- net/core/filter.c | 50 ++++++++++++++++++++++++ 7 files changed, 89 insertions(+), 16 deletions(-)drivers/net/ethernet/mellanox/mlx4/en_netdev.c +6
-6@@ -2622,11 +2622,11 @@ static int mlx4_en_set_tx_maxrate(struct net_device *dev, int queue_index, u32 mreturn err; } -static int mlx4_xdp_set (struct net_device *dev,struct bpf_prog *prog) +static int mlx4_xdp_set (struct net_device *dev,struct xdp_prog *prog) {struct mlx4_en_priv *priv =netdev_priv (dev);struct mlx4_en_dev *mdev = priv->mdev; -struct bpf_prog *old_prog; +struct xdp_prog *old_prog;int xdp_ring_num;int port_up =0 ;int err;@@ -2639,7 +2639,7 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog)*/ if (priv->xdp_ring_num == xdp_ring_num) {if (prog) { - prog =bpf_prog_add (prog, priv->rx_ring_num -1 ); + prog =xdp_prog_add (prog, priv->rx_ring_num);if (IS_ERR (prog))return PTR_ERR (prog); }@@ -2650,7 +2650,7 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog) lockdep_is_held(&mdev->state_lock));rcu_assign_pointer (priv->rx_ring[i]->xdp_prog, prog);if (old_prog) -bpf_prog_put (old_prog); +xdp_prog_put (old_prog); }mutex_unlock (&mdev->state_lock);return 0 ;@@ -2669,7 +2669,7 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog) }if (prog) { - prog =bpf_prog_add (prog, priv->rx_ring_num -1 ); + prog =xdp_prog_add (prog, priv->rx_ring_num);if (IS_ERR (prog))return PTR_ERR (prog); }@@ -2690,7 +2690,7 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog) lockdep_is_held(&mdev->state_lock));rcu_assign_pointer (priv->rx_ring[i]->xdp_prog, prog);if (old_prog) -bpf_prog_put (old_prog); +xdp_prog_put (old_prog); }if (port_up) {drivers/net/ethernet/mellanox/mlx4/en_rx.c +6
-4@@ -535,13 +535,13 @@ void mlx4_en_destroy_rx_ring(struct mlx4_en_priv *priv, {struct mlx4_en_dev *mdev = priv->mdev;struct mlx4_en_rx_ring *ring = *pring; -struct bpf_prog *old_prog; +struct xdp_prog *old_prog; old_prog =rcu_dereference_protected ( ring->xdp_prog,lockdep_is_held (&mdev->state_lock));if (old_prog) -bpf_prog_put (old_prog); +xdp_prog_put (old_prog);mlx4_free_hwq_res (mdev->dev, &ring->wqres, size * stride +TXBB_SIZE );vfree (ring->rx_info); ring->rx_info =NULL ;@@ -783,7 +783,8 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int budstruct mlx4_en_rx_ring *ring = priv->rx_ring[cq->ring];struct mlx4_en_rx_alloc *frags;struct mlx4_en_rx_desc *rx_desc; -struct bpf_prog *xdp_prog; +struct xdp_prog *xdp_prog; +struct bpf_prog *bpf_prog;int doorbell_pending;struct sk_buff *skb;int tx_index;@@ -805,6 +806,7 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud/* Protect accesses to: ring->xdp_prog, priv->mac_hash list */ rcu_read_lock (); xdp_prog =rcu_dereference (ring->xdp_prog); + bpf_prog =rcu_dereference (xdp_prog->bpf); doorbell_pending =0 ; tx_index = (priv->tx_ring_num - priv->xdp_ring_num) + cq->ring;@@ -897,7 +899,7 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud frags[0 ].page_offset; xdp.data_end = xdp.data + length; - act =bpf_prog_run_xdp (xdp_prog, &xdp); + act =bpf_prog_run_xdp (bpf_prog, &xdp);switch (act) {case XDP_PASS :break ;drivers/net/ethernet/mellanox/mlx4/mlx4_en.h +1
-1@@ -340,7 +340,7 @@ struct mlx4_en_rx_ring {u8 fcs_del;void *buf;void *rx_info; -struct bpf_prog __rcu *xdp_prog; +struct xdp_prog __rcu *xdp_prog;struct mlx4_en_page_cache page_cache;unsigned long bytes;unsigned long packets;include/linux/filter.h +14
-0@@ -437,6 +437,20 @@ struct xdp_buff {void *data_end; }; +struct xdp_prog { +u64 flags; +struct bpf_prog *bpf; +atomic_t refcnt; +struct rcu_head rcu;// Do we need RCU freeing? likely right? + +/* Data associated with XDP program goes here */ + +}____cacheline_aligned_in_smp ; + +struct xdp_prog *xdp_prog_alloc (struct bpf_prog *bpf); +struct xdp_prog *xdp_prog_add (struct xdp_prog *xdp,int users); +void xdp_prog_put (struct xdp_prog *prog); +/* compute the linear packet data range [data, data_end) which * will be accessed by cls_bpf and act_bpf programs */ include/linux/netdevice.h +1
-1@@ -825,7 +825,7 @@ struct netdev_xdp {enum xdp_netdev_command command;union {/* XDP_SETUP_PROG */ -struct bpf_prog *prog; +struct xdp_prog *prog;/* XDP_QUERY_PROG */ bool prog_attached; };net/core/dev.c +11
-4@@ -6643,23 +6643,30 @@ EXPORT_SYMBOL(dev_change_proto_down);int dev_change_xdp_fd (struct net_device *dev,int fd) {const struct net_device_ops *ops = dev->netdev_ops; -struct bpf_prog *prog =NULL ; +struct xdp_prog *prog =NULL ; +struct bpf_prog *bpf;struct netdev_xdp xdp = {};int err;if (!ops->ndo_xdp)return -EOPNOTSUPP ;if (fd >=0 ) { - prog =bpf_prog_get_type (fd,BPF_PROG_TYPE_XDP ); -if (IS_ERR (prog)) + bpf =bpf_prog_get_type (fd,BPF_PROG_TYPE_XDP );/* inc refcnt */ +if (IS_ERR (bpf)) +return PTR_ERR (bpf); + + prog =xdp_prog_alloc (bpf); +if (IS_ERR (prog)) { +bpf_prog_put (prog->bpf);return PTR_ERR (prog); + } } xdp.command =XDP_SETUP_PROG ; xdp.prog = prog; err = ops->ndo_xdp (dev, &xdp);if (err <0 && prog) -bpf_prog_put (prog); +bpf_prog_put (prog->bpf);return err; }net/core/filter.c +50
-0@@ -2912,3 +2912,53 @@ out:release_sock (sk);return ret; } + +struct xdp_prog *xdp_prog_alloc (struct bpf_prog *bpf) +{ +struct xdp_prog *xdp; + + xdp =kzalloc (sizeof (*xdp),GFP_KERNEL ); +if (!xdp) +return ERR_PTR (-ENOMEM ); + +/* Note dev_change_xdp_fd() already refcnt inc on bpf prog */ + xdp->bpf = bpf; + +return xdp; +} +EXPORT_SYMBOL (xdp_prog_alloc); + +struct xdp_prog *xdp_prog_add (struct xdp_prog *xdp,int users) +{ +struct bpf_prog *bpf; + + bpf =bpf_prog_add (xdp->bpf, users); +if (IS_ERR (bpf)) { +return (void *)bpf;/* it is already a PTR_ERR */ + } +atomic_add (users, &xdp->refcnt); + +return xdp; +} +EXPORT_SYMBOL (xdp_prog_add); + +void __xdp_prog_put_rcu (struct rcu_head *rcu) +{ + struct xdp_prog *xdp = container_of(rcu, struct xdp_prog, rcu); + +/* Release reference to (future) xdp_port_table here */ + +/* Release refcnt from dev_change_xdp_fd() calling bpf_prog_get_type()*/ +bpf_prog_put (xdp->bpf); + +kfree (xdp); +} + +void xdp_prog_put (struct xdp_prog *prog) +{ +bpf_prog_put (prog->bpf); +if (atomic_dec_and_test (&prog->refcnt)) { +call_rcu (&prog->rcu, __xdp_prog_put_rcu); + } +} +EXPORT_SYMBOL (xdp_prog_put);