| Message ID | 930b1ad3d84f7ca5a41ba75571f9146a932c5394.1646755129.git.lorenzo@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | introduce xdp frags support to veth driver | expand |
Lorenzo Bianconi <lorenzo@kernel.org> writes: > Allow increasing the MTU over page boundaries on veth devices > if the attached xdp program declares to support xdp fragments. > Enable NETIF_F_ALL_TSO when the device is running in xdp mode. > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > --- > drivers/net/veth.c | 28 +++++++++++++++++----------- > 1 file changed, 17 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/veth.c b/drivers/net/veth.c > index 47b21b1d2fd9..c5a2dc2b2e4b 100644 > --- a/drivers/net/veth.c > +++ b/drivers/net/veth.c > @@ -293,8 +293,7 @@ static int veth_forward_skb(struct net_device *dev, struct sk_buff *skb, > /* return true if the specified skb has chances of GRO aggregation > * Don't strive for accuracy, but try to avoid GRO overhead in the most > * common scenarios. > - * When XDP is enabled, all traffic is considered eligible, as the xmit > - * device has TSO off. > + * When XDP is enabled, all traffic is considered eligible. > * When TSO is enabled on the xmit device, we are likely interested only > * in UDP aggregation, explicitly check for that if the skb is suspected > * - the sock_wfree destructor is used by UDP, ICMP and XDP sockets - > @@ -302,11 +301,13 @@ static int veth_forward_skb(struct net_device *dev, struct sk_buff *skb, > */ > static bool veth_skb_is_eligible_for_gro(const struct net_device *dev, > const struct net_device *rcv, > + const struct veth_rq *rq, > const struct sk_buff *skb) > { > - return !(dev->features & NETIF_F_ALL_TSO) || > - (skb->destructor == sock_wfree && > - rcv->features & (NETIF_F_GRO_FRAGLIST | NETIF_F_GRO_UDP_FWD)); > + return rcu_access_pointer(rq->xdp_prog) || > + !(dev->features & NETIF_F_ALL_TSO) || > + (skb->destructor == sock_wfree && > + rcv->features & (NETIF_F_GRO_FRAGLIST | NETIF_F_GRO_UDP_FWD)); > } > > static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev) > @@ -335,7 +336,7 @@ static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev) > * Don't bother with napi/GRO if the skb can't be aggregated > */ > use_napi = rcu_access_pointer(rq->napi) && > - veth_skb_is_eligible_for_gro(dev, rcv, skb); > + veth_skb_is_eligible_for_gro(dev, rcv, rq, skb); > } > > skb_tx_timestamp(skb); > @@ -1525,9 +1526,14 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog, > goto err; > } > > - max_mtu = PAGE_SIZE - VETH_XDP_HEADROOM - > - peer->hard_header_len - > - SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); > + max_mtu = SKB_WITH_OVERHEAD(PAGE_SIZE - VETH_XDP_HEADROOM) - > + peer->hard_header_len; Why are we no longer accounting the size of the skb_shared_info if the program doesn't support frags? > + /* Allow increasing the max_mtu if the program supports > + * XDP fragments. > + */ > + if (prog->aux->xdp_has_frags) > + max_mtu += PAGE_SIZE * MAX_SKB_FRAGS; > + > if (peer->mtu > max_mtu) { > NL_SET_ERR_MSG_MOD(extack, "Peer MTU is too large to set XDP"); > err = -ERANGE; > @@ -1549,7 +1555,7 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog, > } > > if (!old_prog) { > - peer->hw_features &= ~NETIF_F_GSO_SOFTWARE; > + peer->hw_features &= ~NETIF_F_GSO_FRAGLIST; The patch description says we're enabling TSO, but this change enables a couple of other flags as well. Also, it's not quite obvious to me why your change makes this possible? Is it because we can now execute XDP on a full TSO packet at once? Because then this should be coupled to the xdp_has_frags flag of the XDP program? Or will the TSO packet be segmented before it hits the XDP program? But then this change has nothing to do with the rest of your series? Please also add this explanation to the commit message :) -Toke
> Lorenzo Bianconi <lorenzo@kernel.org> writes: > > > Allow increasing the MTU over page boundaries on veth devices > > if the attached xdp program declares to support xdp fragments. > > Enable NETIF_F_ALL_TSO when the device is running in xdp mode. > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > --- > > drivers/net/veth.c | 28 +++++++++++++++++----------- > > 1 file changed, 17 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/net/veth.c b/drivers/net/veth.c > > index 47b21b1d2fd9..c5a2dc2b2e4b 100644 > > --- a/drivers/net/veth.c > > +++ b/drivers/net/veth.c > > @@ -293,8 +293,7 @@ static int veth_forward_skb(struct net_device *dev, struct sk_buff *skb, > > /* return true if the specified skb has chances of GRO aggregation > > * Don't strive for accuracy, but try to avoid GRO overhead in the most > > * common scenarios. > > - * When XDP is enabled, all traffic is considered eligible, as the xmit > > - * device has TSO off. > > + * When XDP is enabled, all traffic is considered eligible. > > * When TSO is enabled on the xmit device, we are likely interested only > > * in UDP aggregation, explicitly check for that if the skb is suspected > > * - the sock_wfree destructor is used by UDP, ICMP and XDP sockets - > > @@ -302,11 +301,13 @@ static int veth_forward_skb(struct net_device *dev, struct sk_buff *skb, > > */ > > static bool veth_skb_is_eligible_for_gro(const struct net_device *dev, > > const struct net_device *rcv, > > + const struct veth_rq *rq, > > const struct sk_buff *skb) > > { > > - return !(dev->features & NETIF_F_ALL_TSO) || > > - (skb->destructor == sock_wfree && > > - rcv->features & (NETIF_F_GRO_FRAGLIST | NETIF_F_GRO_UDP_FWD)); > > + return rcu_access_pointer(rq->xdp_prog) || > > + !(dev->features & NETIF_F_ALL_TSO) || > > + (skb->destructor == sock_wfree && > > + rcv->features & (NETIF_F_GRO_FRAGLIST | NETIF_F_GRO_UDP_FWD)); > > } > > > > static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev) > > @@ -335,7 +336,7 @@ static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev) > > * Don't bother with napi/GRO if the skb can't be aggregated > > */ > > use_napi = rcu_access_pointer(rq->napi) && > > - veth_skb_is_eligible_for_gro(dev, rcv, skb); > > + veth_skb_is_eligible_for_gro(dev, rcv, rq, skb); > > } > > > > skb_tx_timestamp(skb); > > @@ -1525,9 +1526,14 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog, > > goto err; > > } > > > > - max_mtu = PAGE_SIZE - VETH_XDP_HEADROOM - > > - peer->hard_header_len - > > - SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); > > + max_mtu = SKB_WITH_OVERHEAD(PAGE_SIZE - VETH_XDP_HEADROOM) - > > + peer->hard_header_len; > > Why are we no longer accounting the size of the skb_shared_info if the > program doesn't support frags? doing so we do not allow packets over page boundaries (so non-linear xdp_buff) if the attached program does not delclare to support them, right? > > > + /* Allow increasing the max_mtu if the program supports > > + * XDP fragments. > > + */ > > + if (prog->aux->xdp_has_frags) > > + max_mtu += PAGE_SIZE * MAX_SKB_FRAGS; > > + > > if (peer->mtu > max_mtu) { > > NL_SET_ERR_MSG_MOD(extack, "Peer MTU is too large to set XDP"); > > err = -ERANGE; > > @@ -1549,7 +1555,7 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog, > > } > > > > if (!old_prog) { > > - peer->hw_features &= ~NETIF_F_GSO_SOFTWARE; > > + peer->hw_features &= ~NETIF_F_GSO_FRAGLIST; > > The patch description says we're enabling TSO, but this change enables a > couple of other flags as well. Also, it's not quite obvious to me why > your change makes this possible? Is it because we can now execute XDP on > a full TSO packet at once? Because then this should be coupled to the > xdp_has_frags flag of the XDP program? Or will the TSO packet be > segmented before it hits the XDP program? But then this change has > nothing to do with the rest of your series? actually tso support is not mandatory for this feature (even if it is probably meaningful). I will drop it from v5 and we can take care of it in a susequent patch. Regards, Lorenzo > > Please also add this explanation to the commit message :) > > -Toke >
Lorenzo Bianconi <lorenzo.bianconi@redhat.com> writes: >> Lorenzo Bianconi <lorenzo@kernel.org> writes: >> >> > Allow increasing the MTU over page boundaries on veth devices >> > if the attached xdp program declares to support xdp fragments. >> > Enable NETIF_F_ALL_TSO when the device is running in xdp mode. >> > >> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> >> > --- >> > drivers/net/veth.c | 28 +++++++++++++++++----------- >> > 1 file changed, 17 insertions(+), 11 deletions(-) >> > >> > diff --git a/drivers/net/veth.c b/drivers/net/veth.c >> > index 47b21b1d2fd9..c5a2dc2b2e4b 100644 >> > --- a/drivers/net/veth.c >> > +++ b/drivers/net/veth.c >> > @@ -293,8 +293,7 @@ static int veth_forward_skb(struct net_device *dev, struct sk_buff *skb, >> > /* return true if the specified skb has chances of GRO aggregation >> > * Don't strive for accuracy, but try to avoid GRO overhead in the most >> > * common scenarios. >> > - * When XDP is enabled, all traffic is considered eligible, as the xmit >> > - * device has TSO off. >> > + * When XDP is enabled, all traffic is considered eligible. >> > * When TSO is enabled on the xmit device, we are likely interested only >> > * in UDP aggregation, explicitly check for that if the skb is suspected >> > * - the sock_wfree destructor is used by UDP, ICMP and XDP sockets - >> > @@ -302,11 +301,13 @@ static int veth_forward_skb(struct net_device *dev, struct sk_buff *skb, >> > */ >> > static bool veth_skb_is_eligible_for_gro(const struct net_device *dev, >> > const struct net_device *rcv, >> > + const struct veth_rq *rq, >> > const struct sk_buff *skb) >> > { >> > - return !(dev->features & NETIF_F_ALL_TSO) || >> > - (skb->destructor == sock_wfree && >> > - rcv->features & (NETIF_F_GRO_FRAGLIST | NETIF_F_GRO_UDP_FWD)); >> > + return rcu_access_pointer(rq->xdp_prog) || >> > + !(dev->features & NETIF_F_ALL_TSO) || >> > + (skb->destructor == sock_wfree && >> > + rcv->features & (NETIF_F_GRO_FRAGLIST | NETIF_F_GRO_UDP_FWD)); >> > } >> > >> > static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev) >> > @@ -335,7 +336,7 @@ static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev) >> > * Don't bother with napi/GRO if the skb can't be aggregated >> > */ >> > use_napi = rcu_access_pointer(rq->napi) && >> > - veth_skb_is_eligible_for_gro(dev, rcv, skb); >> > + veth_skb_is_eligible_for_gro(dev, rcv, rq, skb); >> > } >> > >> > skb_tx_timestamp(skb); >> > @@ -1525,9 +1526,14 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog, >> > goto err; >> > } >> > >> > - max_mtu = PAGE_SIZE - VETH_XDP_HEADROOM - >> > - peer->hard_header_len - >> > - SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); >> > + max_mtu = SKB_WITH_OVERHEAD(PAGE_SIZE - VETH_XDP_HEADROOM) - >> > + peer->hard_header_len; >> >> Why are we no longer accounting the size of the skb_shared_info if the >> program doesn't support frags? > > doing so we do not allow packets over page boundaries (so non-linear xdp_buff) > if the attached program does not delclare to support them, right? Oh, sorry, somehow completely skipped over the addition of the SKB_WITH_OVERHEAD() - so thought you were removing the sizeof(struct skb_shared_info) from the calculation... >> > + /* Allow increasing the max_mtu if the program supports >> > + * XDP fragments. >> > + */ >> > + if (prog->aux->xdp_has_frags) >> > + max_mtu += PAGE_SIZE * MAX_SKB_FRAGS; >> > + >> > if (peer->mtu > max_mtu) { >> > NL_SET_ERR_MSG_MOD(extack, "Peer MTU is too large to set XDP"); >> > err = -ERANGE; >> > @@ -1549,7 +1555,7 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog, >> > } >> > >> > if (!old_prog) { >> > - peer->hw_features &= ~NETIF_F_GSO_SOFTWARE; >> > + peer->hw_features &= ~NETIF_F_GSO_FRAGLIST; >> >> The patch description says we're enabling TSO, but this change enables a >> couple of other flags as well. Also, it's not quite obvious to me why >> your change makes this possible? Is it because we can now execute XDP on >> a full TSO packet at once? Because then this should be coupled to the >> xdp_has_frags flag of the XDP program? Or will the TSO packet be >> segmented before it hits the XDP program? But then this change has >> nothing to do with the rest of your series? > > actually tso support is not mandatory for this feature (even if it is probably > meaningful). I will drop it from v5 and we can take care of it in a susequent > patch. OK, SGTM -Toke
diff --git a/drivers/net/veth.c b/drivers/net/veth.c index 47b21b1d2fd9..c5a2dc2b2e4b 100644 --- a/drivers/net/veth.c +++ b/drivers/net/veth.c @@ -293,8 +293,7 @@ static int veth_forward_skb(struct net_device *dev, struct sk_buff *skb, /* return true if the specified skb has chances of GRO aggregation * Don't strive for accuracy, but try to avoid GRO overhead in the most * common scenarios. - * When XDP is enabled, all traffic is considered eligible, as the xmit - * device has TSO off. + * When XDP is enabled, all traffic is considered eligible. * When TSO is enabled on the xmit device, we are likely interested only * in UDP aggregation, explicitly check for that if the skb is suspected * - the sock_wfree destructor is used by UDP, ICMP and XDP sockets - @@ -302,11 +301,13 @@ static int veth_forward_skb(struct net_device *dev, struct sk_buff *skb, */ static bool veth_skb_is_eligible_for_gro(const struct net_device *dev, const struct net_device *rcv, + const struct veth_rq *rq, const struct sk_buff *skb) { - return !(dev->features & NETIF_F_ALL_TSO) || - (skb->destructor == sock_wfree && - rcv->features & (NETIF_F_GRO_FRAGLIST | NETIF_F_GRO_UDP_FWD)); + return rcu_access_pointer(rq->xdp_prog) || + !(dev->features & NETIF_F_ALL_TSO) || + (skb->destructor == sock_wfree && + rcv->features & (NETIF_F_GRO_FRAGLIST | NETIF_F_GRO_UDP_FWD)); } static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev) @@ -335,7 +336,7 @@ static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev) * Don't bother with napi/GRO if the skb can't be aggregated */ use_napi = rcu_access_pointer(rq->napi) && - veth_skb_is_eligible_for_gro(dev, rcv, skb); + veth_skb_is_eligible_for_gro(dev, rcv, rq, skb); } skb_tx_timestamp(skb); @@ -1525,9 +1526,14 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog, goto err; } - max_mtu = PAGE_SIZE - VETH_XDP_HEADROOM - - peer->hard_header_len - - SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); + max_mtu = SKB_WITH_OVERHEAD(PAGE_SIZE - VETH_XDP_HEADROOM) - + peer->hard_header_len; + /* Allow increasing the max_mtu if the program supports + * XDP fragments. + */ + if (prog->aux->xdp_has_frags) + max_mtu += PAGE_SIZE * MAX_SKB_FRAGS; + if (peer->mtu > max_mtu) { NL_SET_ERR_MSG_MOD(extack, "Peer MTU is too large to set XDP"); err = -ERANGE; @@ -1549,7 +1555,7 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog, } if (!old_prog) { - peer->hw_features &= ~NETIF_F_GSO_SOFTWARE; + peer->hw_features &= ~NETIF_F_GSO_FRAGLIST; peer->max_mtu = max_mtu; } } @@ -1560,7 +1566,7 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog, veth_disable_xdp(dev); if (peer) { - peer->hw_features |= NETIF_F_GSO_SOFTWARE; + peer->hw_features |= NETIF_F_GSO_FRAGLIST; peer->max_mtu = ETH_MAX_MTU; } }
Allow increasing the MTU over page boundaries on veth devices if the attached xdp program declares to support xdp fragments. Enable NETIF_F_ALL_TSO when the device is running in xdp mode. Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> --- drivers/net/veth.c | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-)