| Message ID | 20240412152120.115067-2-richardbgobert@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: gro: add flush/flush_id checks and fix wrong offset in udp | expand |
Richard Gobert wrote: > GRO-GSO path is supposed to be transparent and as such L3 flush checks are > relevant to all flows which call skb_gro_receive. This patch uses the same > logic and code from tcp_gro_receive but in the relevant flow path in > udp_gro_receive_segment. > > Fixes: 36707061d6ba ("udp: allow forwarding of plain (non-fraglisted) UDP GRO packets") > Signed-off-by: Richard Gobert <richardbgobert@gmail.com> Reviewed-by: Willem de Bruijn <willemb@google.com> > --- > net/ipv4/udp_offload.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c > index 3498dd1d0694..1f4e08f43c4b 100644 > --- a/net/ipv4/udp_offload.c > +++ b/net/ipv4/udp_offload.c > @@ -471,6 +471,7 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head, > struct sk_buff *p; > unsigned int ulen; > int ret = 0; > + int flush; > > /* requires non zero csum, for symmetry with GSO */ > if (!uh->check) { > @@ -528,7 +529,17 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head, > skb_gro_postpull_rcsum(skb, uh, > sizeof(struct udphdr)); > > - ret = skb_gro_receive(p, skb); > + flush = NAPI_GRO_CB(p)->flush; > + > + if (NAPI_GRO_CB(p)->flush_id != 1 || > + NAPI_GRO_CB(p)->count != 1 || > + !NAPI_GRO_CB(p)->is_atomic) > + flush |= NAPI_GRO_CB(p)->flush_id; > + else > + NAPI_GRO_CB(p)->is_atomic = false; > + > + if (flush || skb_gro_receive(p, skb)) > + ret = 1; UDP_L4 does not have the SKB_GSO_TCP_FIXEDID that uses is_atomic as input. And I still don't fully internalize the flush_id logic after staring at it for more than one coffee. But even ignoring those, the flush signal of NAPI_GRO_CB(p)->flush set the network layer must be followed, so ACK. Thanks for the fix.
On Sat, Apr 13, 2024 at 11:38 AM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > Richard Gobert wrote: > > GRO-GSO path is supposed to be transparent and as such L3 flush checks are > > relevant to all flows which call skb_gro_receive. This patch uses the same > > logic and code from tcp_gro_receive but in the relevant flow path in > > udp_gro_receive_segment. > > > > Fixes: 36707061d6ba ("udp: allow forwarding of plain (non-fraglisted) UDP GRO packets") > > Signed-off-by: Richard Gobert <richardbgobert@gmail.com> > > Reviewed-by: Willem de Bruijn <willemb@google.com> > > > --- > > net/ipv4/udp_offload.c | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c > > index 3498dd1d0694..1f4e08f43c4b 100644 > > --- a/net/ipv4/udp_offload.c > > +++ b/net/ipv4/udp_offload.c > > @@ -471,6 +471,7 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head, > > struct sk_buff *p; > > unsigned int ulen; > > int ret = 0; > > + int flush; > > > > /* requires non zero csum, for symmetry with GSO */ > > if (!uh->check) { > > @@ -528,7 +529,17 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head, > > skb_gro_postpull_rcsum(skb, uh, > > sizeof(struct udphdr)); > > > > - ret = skb_gro_receive(p, skb); > > + flush = NAPI_GRO_CB(p)->flush; > > + > > + if (NAPI_GRO_CB(p)->flush_id != 1 || > > + NAPI_GRO_CB(p)->count != 1 || > > + !NAPI_GRO_CB(p)->is_atomic) > > + flush |= NAPI_GRO_CB(p)->flush_id; > > + else > > + NAPI_GRO_CB(p)->is_atomic = false; > > + > > + if (flush || skb_gro_receive(p, skb)) > > + ret = 1; > > UDP_L4 does not have the SKB_GSO_TCP_FIXEDID that uses is_atomic as > input. > > And I still don't fully internalize the flush_id logic after staring > at it for more than one coffee. The flush_id field is there to indicate the difference between the current IPv4 ID of the previous IP header. It is meant to be used in conjunction with the is_atomic for the frame coalescing. Basically after the second frame we can decide the pattern either incrementing IPv4 ID or fixed, so on frames 3 or later we can decide to drop the frame if it doesn't follow that pattern. > But even ignoring those, the flush signal of NAPI_GRO_CB(p)->flush > set the network layer must be followed, so ACK. Thanks for the fix. I'm not sure about the placement of this code though. That is the one thing that seems off to me. Specifically this seems like it should be done before we start the postpull, not after. It should be something that can terminate the flow before we attempt to aggregate the UDP headers.
Alexander Duyck wrote: > On Sat, Apr 13, 2024 at 11:38 AM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > > > > Richard Gobert wrote: > > > GRO-GSO path is supposed to be transparent and as such L3 flush checks are > > > relevant to all flows which call skb_gro_receive. This patch uses the same > > > logic and code from tcp_gro_receive but in the relevant flow path in > > > udp_gro_receive_segment. > > > > > > Fixes: 36707061d6ba ("udp: allow forwarding of plain (non-fraglisted) UDP GRO packets") > > > Signed-off-by: Richard Gobert <richardbgobert@gmail.com> > > > > Reviewed-by: Willem de Bruijn <willemb@google.com> > > > > > --- > > > net/ipv4/udp_offload.c | 13 ++++++++++++- > > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > > > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c > > > index 3498dd1d0694..1f4e08f43c4b 100644 > > > --- a/net/ipv4/udp_offload.c > > > +++ b/net/ipv4/udp_offload.c > > > @@ -471,6 +471,7 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head, > > > struct sk_buff *p; > > > unsigned int ulen; > > > int ret = 0; > > > + int flush; > > > > > > /* requires non zero csum, for symmetry with GSO */ > > > if (!uh->check) { > > > @@ -528,7 +529,17 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head, > > > skb_gro_postpull_rcsum(skb, uh, > > > sizeof(struct udphdr)); > > > > > > - ret = skb_gro_receive(p, skb); > > > + flush = NAPI_GRO_CB(p)->flush; > > > + > > > + if (NAPI_GRO_CB(p)->flush_id != 1 || > > > + NAPI_GRO_CB(p)->count != 1 || > > > + !NAPI_GRO_CB(p)->is_atomic) > > > + flush |= NAPI_GRO_CB(p)->flush_id; > > > + else > > > + NAPI_GRO_CB(p)->is_atomic = false; > > > + > > > + if (flush || skb_gro_receive(p, skb)) > > > + ret = 1; > > > > UDP_L4 does not have the SKB_GSO_TCP_FIXEDID that uses is_atomic as > > input. > > > > And I still don't fully internalize the flush_id logic after staring > > at it for more than one coffee. > > The flush_id field is there to indicate the difference between the > current IPv4 ID of the previous IP header. It is meant to be used in > conjunction with the is_atomic for the frame coalescing. Basically > after the second frame we can decide the pattern either incrementing > IPv4 ID or fixed, so on frames 3 or later we can decide to drop the > frame if it doesn't follow that pattern. > > > But even ignoring those, the flush signal of NAPI_GRO_CB(p)->flush > > set the network layer must be followed, so ACK. Thanks for the fix. > > I'm not sure about the placement of this code though. That is the one > thing that seems off to me. Specifically this seems like it should be > done before we start the postpull, not after. It should be something > that can terminate the flow before we attempt to aggregate the UDP > headers. In principle agreed that we should conclude the flush checks before doing prep for coalescing. In practice it does not matter? NAPI_GRO_CB(skb)->csum will be ignored if the packet gets flushed.
On Mon, Apr 15, 2024 at 8:00 AM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > Alexander Duyck wrote: > > On Sat, Apr 13, 2024 at 11:38 AM Willem de Bruijn > > <willemdebruijn.kernel@gmail.com> wrote: > > > > > > Richard Gobert wrote: > > > > GRO-GSO path is supposed to be transparent and as such L3 flush checks are > > > > relevant to all flows which call skb_gro_receive. This patch uses the same > > > > logic and code from tcp_gro_receive but in the relevant flow path in > > > > udp_gro_receive_segment. > > > > > > > > Fixes: 36707061d6ba ("udp: allow forwarding of plain (non-fraglisted) UDP GRO packets") > > > > Signed-off-by: Richard Gobert <richardbgobert@gmail.com> > > > > > > Reviewed-by: Willem de Bruijn <willemb@google.com> > > > > > > > --- > > > > net/ipv4/udp_offload.c | 13 ++++++++++++- > > > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c > > > > index 3498dd1d0694..1f4e08f43c4b 100644 > > > > --- a/net/ipv4/udp_offload.c > > > > +++ b/net/ipv4/udp_offload.c > > > > @@ -471,6 +471,7 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head, > > > > struct sk_buff *p; > > > > unsigned int ulen; > > > > int ret = 0; > > > > + int flush; > > > > > > > > /* requires non zero csum, for symmetry with GSO */ > > > > if (!uh->check) { > > > > @@ -528,7 +529,17 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head, > > > > skb_gro_postpull_rcsum(skb, uh, > > > > sizeof(struct udphdr)); > > > > > > > > - ret = skb_gro_receive(p, skb); > > > > + flush = NAPI_GRO_CB(p)->flush; > > > > + > > > > + if (NAPI_GRO_CB(p)->flush_id != 1 || > > > > + NAPI_GRO_CB(p)->count != 1 || > > > > + !NAPI_GRO_CB(p)->is_atomic) > > > > + flush |= NAPI_GRO_CB(p)->flush_id; > > > > + else > > > > + NAPI_GRO_CB(p)->is_atomic = false; > > > > + > > > > + if (flush || skb_gro_receive(p, skb)) > > > > + ret = 1; > > > > > > UDP_L4 does not have the SKB_GSO_TCP_FIXEDID that uses is_atomic as > > > input. > > > > > > And I still don't fully internalize the flush_id logic after staring > > > at it for more than one coffee. > > > > The flush_id field is there to indicate the difference between the > > current IPv4 ID of the previous IP header. It is meant to be used in > > conjunction with the is_atomic for the frame coalescing. Basically > > after the second frame we can decide the pattern either incrementing > > IPv4 ID or fixed, so on frames 3 or later we can decide to drop the > > frame if it doesn't follow that pattern. > > > > > But even ignoring those, the flush signal of NAPI_GRO_CB(p)->flush > > > set the network layer must be followed, so ACK. Thanks for the fix. > > > > I'm not sure about the placement of this code though. That is the one > > thing that seems off to me. Specifically this seems like it should be > > done before we start the postpull, not after. It should be something > > that can terminate the flow before we attempt to aggregate the UDP > > headers. > > In principle agreed that we should conclude the flush checks before > doing prep for coalescing. > > In practice it does not matter? NAPI_GRO_CB(skb)->csum will be ignored > if the packet gets flushed. I was referring more to the fact that this code is one of two branches. So there is this path, and then the is_flist branch that comes before this. I would think this logic would apply to both wouldn't it? I am not familiar with the code so I cannot say for certain if it does or doesn't. If it doesn't then yes. I suppose it doesn't matter.
Alexander Duyck wrote: > On Mon, Apr 15, 2024 at 8:00 AM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > > > > Alexander Duyck wrote: > > > On Sat, Apr 13, 2024 at 11:38 AM Willem de Bruijn > > > <willemdebruijn.kernel@gmail.com> wrote: > > > > > > > > Richard Gobert wrote: > > > > > GRO-GSO path is supposed to be transparent and as such L3 flush checks are > > > > > relevant to all flows which call skb_gro_receive. This patch uses the same > > > > > logic and code from tcp_gro_receive but in the relevant flow path in > > > > > udp_gro_receive_segment. > > > > > > > > > > Fixes: 36707061d6ba ("udp: allow forwarding of plain (non-fraglisted) UDP GRO packets") > > > > > Signed-off-by: Richard Gobert <richardbgobert@gmail.com> > > > > > > > > Reviewed-by: Willem de Bruijn <willemb@google.com> > > > > > > > > > --- > > > > > net/ipv4/udp_offload.c | 13 ++++++++++++- > > > > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c > > > > > index 3498dd1d0694..1f4e08f43c4b 100644 > > > > > --- a/net/ipv4/udp_offload.c > > > > > +++ b/net/ipv4/udp_offload.c > > > > > @@ -471,6 +471,7 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head, > > > > > struct sk_buff *p; > > > > > unsigned int ulen; > > > > > int ret = 0; > > > > > + int flush; > > > > > > > > > > /* requires non zero csum, for symmetry with GSO */ > > > > > if (!uh->check) { > > > > > @@ -528,7 +529,17 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head, > > > > > skb_gro_postpull_rcsum(skb, uh, > > > > > sizeof(struct udphdr)); > > > > > > > > > > - ret = skb_gro_receive(p, skb); > > > > > + flush = NAPI_GRO_CB(p)->flush; > > > > > + > > > > > + if (NAPI_GRO_CB(p)->flush_id != 1 || > > > > > + NAPI_GRO_CB(p)->count != 1 || > > > > > + !NAPI_GRO_CB(p)->is_atomic) > > > > > + flush |= NAPI_GRO_CB(p)->flush_id; > > > > > + else > > > > > + NAPI_GRO_CB(p)->is_atomic = false; > > > > > + > > > > > + if (flush || skb_gro_receive(p, skb)) > > > > > + ret = 1; > > > > > > > > UDP_L4 does not have the SKB_GSO_TCP_FIXEDID that uses is_atomic as > > > > input. > > > > > > > > And I still don't fully internalize the flush_id logic after staring > > > > at it for more than one coffee. > > > > > > The flush_id field is there to indicate the difference between the > > > current IPv4 ID of the previous IP header. It is meant to be used in > > > conjunction with the is_atomic for the frame coalescing. Basically > > > after the second frame we can decide the pattern either incrementing > > > IPv4 ID or fixed, so on frames 3 or later we can decide to drop the > > > frame if it doesn't follow that pattern. > > > > > > > But even ignoring those, the flush signal of NAPI_GRO_CB(p)->flush > > > > set the network layer must be followed, so ACK. Thanks for the fix. > > > > > > I'm not sure about the placement of this code though. That is the one > > > thing that seems off to me. Specifically this seems like it should be > > > done before we start the postpull, not after. It should be something > > > that can terminate the flow before we attempt to aggregate the UDP > > > headers. > > > > In principle agreed that we should conclude the flush checks before > > doing prep for coalescing. > > > > In practice it does not matter? NAPI_GRO_CB(skb)->csum will be ignored > > if the packet gets flushed. > > I was referring more to the fact that this code is one of two > branches. So there is this path, and then the is_flist branch that > comes before this. I would think this logic would apply to both > wouldn't it? I am not familiar with the code so I cannot say for > certain if it does or doesn't. If it doesn't then yes. I suppose it > doesn't matter. With if_flist, all original segments are preserved in the frag_list, so can be sent out as is. Good point that that is no excuse for combining three or more segments where some have a fixed id and others an incrementing id.
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c index 3498dd1d0694..1f4e08f43c4b 100644 --- a/net/ipv4/udp_offload.c +++ b/net/ipv4/udp_offload.c @@ -471,6 +471,7 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head, struct sk_buff *p; unsigned int ulen; int ret = 0; + int flush; /* requires non zero csum, for symmetry with GSO */ if (!uh->check) { @@ -528,7 +529,17 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head, skb_gro_postpull_rcsum(skb, uh, sizeof(struct udphdr)); - ret = skb_gro_receive(p, skb); + flush = NAPI_GRO_CB(p)->flush; + + if (NAPI_GRO_CB(p)->flush_id != 1 || + NAPI_GRO_CB(p)->count != 1 || + !NAPI_GRO_CB(p)->is_atomic) + flush |= NAPI_GRO_CB(p)->flush_id; + else + NAPI_GRO_CB(p)->is_atomic = false; + + if (flush || skb_gro_receive(p, skb)) + ret = 1; } }
GRO-GSO path is supposed to be transparent and as such L3 flush checks are relevant to all flows which call skb_gro_receive. This patch uses the same logic and code from tcp_gro_receive but in the relevant flow path in udp_gro_receive_segment. Fixes: 36707061d6ba ("udp: allow forwarding of plain (non-fraglisted) UDP GRO packets") Signed-off-by: Richard Gobert <richardbgobert@gmail.com> --- net/ipv4/udp_offload.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)