* [PATCH net-next 0/2] net: fix the features flag in sctp_gso_segment @ 2021-01-11 12:45 Xin Long 2021-01-11 12:45 ` [PATCH net-next 1/2] net: move the hsize check to the else block in skb_segment Xin Long 0 siblings, 1 reply; 5+ messages in thread From: Xin Long @ 2021-01-11 12:45 UTC (permalink / raw) To: network dev, linux-sctp Cc: Marcelo Ricardo Leitner, Neil Horman, davem, Jakub Kicinski, Alexander Duyck Patch 1/2 is to improve the code in skb_segment(), and it is needed by Patch 2/2. Xin Long (2): net: move the hsize check to the else block in skb_segment sctp: remove the NETIF_F_SG flag before calling skb_segment net/core/skbuff.c | 5 +++-- net/sctp/offload.c | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) -- 2.1.0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH net-next 1/2] net: move the hsize check to the else block in skb_segment 2021-01-11 12:45 [PATCH net-next 0/2] net: fix the features flag in sctp_gso_segment Xin Long @ 2021-01-11 12:45 ` Xin Long 2021-01-11 12:45 ` [PATCH net-next 2/2] sctp: remove the NETIF_F_SG flag before calling skb_segment Xin Long 2021-01-11 16:26 ` [PATCH net-next 1/2] net: move the hsize check to the else block in skb_segment Alexander Duyck 0 siblings, 2 replies; 5+ messages in thread From: Xin Long @ 2021-01-11 12:45 UTC (permalink / raw) To: network dev, linux-sctp Cc: Marcelo Ricardo Leitner, Neil Horman, davem, Jakub Kicinski, Alexander Duyck After commit 89319d3801d1 ("net: Add frag_list support to skb_segment"), it goes to process frag_list when !hsize in skb_segment(). However, when using skb frag_list, sg normally should not be set. In this case, hsize will be set with len right before !hsize check, then it won't go to frag_list processing code. So the right thing to do is move the hsize check to the else block, so that it won't affect the !hsize check for frag_list processing. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/core/skbuff.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 7626a33..ea79359 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -3855,8 +3855,6 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb, hsize = skb_headlen(head_skb) - offset; if (hsize < 0) hsize = 0; - if (hsize > len || !sg) - hsize = len; if (!hsize && i >= nfrags && skb_headlen(list_skb) && (skb_headlen(list_skb) == len || sg)) { @@ -3901,6 +3899,9 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb, skb_release_head_state(nskb); __skb_push(nskb, doffset); } else { + if (hsize > len || !sg) + hsize = len; + nskb = __alloc_skb(hsize + doffset + headroom, GFP_ATOMIC, skb_alloc_rx_flag(head_skb), NUMA_NO_NODE); -- 2.1.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH net-next 2/2] sctp: remove the NETIF_F_SG flag before calling skb_segment 2021-01-11 12:45 ` [PATCH net-next 1/2] net: move the hsize check to the else block in skb_segment Xin Long @ 2021-01-11 12:45 ` Xin Long 2021-01-11 16:26 ` [PATCH net-next 1/2] net: move the hsize check to the else block in skb_segment Alexander Duyck 1 sibling, 0 replies; 5+ messages in thread From: Xin Long @ 2021-01-11 12:45 UTC (permalink / raw) To: network dev, linux-sctp Cc: Marcelo Ricardo Leitner, Neil Horman, davem, Jakub Kicinski, Alexander Duyck It makes more sense to clear NETIF_F_SG instead of set it when calling skb_segment() in sctp_gso_segment(), since SCTP GSO is using head_skb's fraglist, of which all frags are linear skbs. This will make SCTP GSO code more understandable. Suggested-by: Alexander Duyck <alexander.duyck@gmail.com> Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/sctp/offload.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/sctp/offload.c b/net/sctp/offload.c index ce281a9..eb874e3 100644 --- a/net/sctp/offload.c +++ b/net/sctp/offload.c @@ -68,7 +68,7 @@ static struct sk_buff *sctp_gso_segment(struct sk_buff *skb, goto out; } - segs = skb_segment(skb, features | NETIF_F_HW_CSUM | NETIF_F_SG); + segs = skb_segment(skb, (features | NETIF_F_HW_CSUM) & ~NETIF_F_SG); if (IS_ERR(segs)) goto out; -- 2.1.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next 1/2] net: move the hsize check to the else block in skb_segment 2021-01-11 12:45 ` [PATCH net-next 1/2] net: move the hsize check to the else block in skb_segment Xin Long 2021-01-11 12:45 ` [PATCH net-next 2/2] sctp: remove the NETIF_F_SG flag before calling skb_segment Xin Long @ 2021-01-11 16:26 ` Alexander Duyck 2021-01-12 7:20 ` Xin Long 1 sibling, 1 reply; 5+ messages in thread From: Alexander Duyck @ 2021-01-11 16:26 UTC (permalink / raw) To: Xin Long Cc: network dev, linux-sctp @ vger . kernel . org, Marcelo Ricardo Leitner, Neil Horman, David Miller, Jakub Kicinski On Mon, Jan 11, 2021 at 4:45 AM Xin Long <lucien.xin@gmail.com> wrote: > > After commit 89319d3801d1 ("net: Add frag_list support to skb_segment"), > it goes to process frag_list when !hsize in skb_segment(). However, when > using skb frag_list, sg normally should not be set. In this case, hsize > will be set with len right before !hsize check, then it won't go to > frag_list processing code. > > So the right thing to do is move the hsize check to the else block, so > that it won't affect the !hsize check for frag_list processing. > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > --- > net/core/skbuff.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index 7626a33..ea79359 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -3855,8 +3855,6 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb, > hsize = skb_headlen(head_skb) - offset; > if (hsize < 0) > hsize = 0; > - if (hsize > len || !sg) > - hsize = len; > > if (!hsize && i >= nfrags && skb_headlen(list_skb) && > (skb_headlen(list_skb) == len || sg)) { So looking at the function it seems like the only spot where the standard path actually reads the hsize value is right here, and it is overwritten before we exit the non-error portion of the if statement. I wonder if we couldn't save ourselves a few cycles and avoid an unnecessary assignment by replacing the "!hsize" with a check for "hsize <= 0" and just move the entire set of checks above down into the lower block. > @@ -3901,6 +3899,9 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb, > skb_release_head_state(nskb); > __skb_push(nskb, doffset); > } else { > + if (hsize > len || !sg) > + hsize = len; > + Then you could essentially just add the "if (hsize < 0)" piece here as an "else if" check and avoid the check if it isn't needed. > nskb = __alloc_skb(hsize + doffset + headroom, > GFP_ATOMIC, skb_alloc_rx_flag(head_skb), > NUMA_NO_NODE); > -- > 2.1.0 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next 1/2] net: move the hsize check to the else block in skb_segment 2021-01-11 16:26 ` [PATCH net-next 1/2] net: move the hsize check to the else block in skb_segment Alexander Duyck @ 2021-01-12 7:20 ` Xin Long 0 siblings, 0 replies; 5+ messages in thread From: Xin Long @ 2021-01-12 7:20 UTC (permalink / raw) To: Alexander Duyck Cc: network dev, linux-sctp @ vger . kernel . org, Marcelo Ricardo Leitner, Neil Horman, David Miller, Jakub Kicinski On Tue, Jan 12, 2021 at 12:26 AM Alexander Duyck <alexander.duyck@gmail.com> wrote: > > On Mon, Jan 11, 2021 at 4:45 AM Xin Long <lucien.xin@gmail.com> wrote: > > > > After commit 89319d3801d1 ("net: Add frag_list support to skb_segment"), > > it goes to process frag_list when !hsize in skb_segment(). However, when > > using skb frag_list, sg normally should not be set. In this case, hsize > > will be set with len right before !hsize check, then it won't go to > > frag_list processing code. > > > > So the right thing to do is move the hsize check to the else block, so > > that it won't affect the !hsize check for frag_list processing. > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > --- > > net/core/skbuff.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > > index 7626a33..ea79359 100644 > > --- a/net/core/skbuff.c > > +++ b/net/core/skbuff.c > > @@ -3855,8 +3855,6 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb, > > hsize = skb_headlen(head_skb) - offset; > > if (hsize < 0) > > hsize = 0; > > - if (hsize > len || !sg) > > - hsize = len; > > > > if (!hsize && i >= nfrags && skb_headlen(list_skb) && > > (skb_headlen(list_skb) == len || sg)) { > > So looking at the function it seems like the only spot where the > standard path actually reads the hsize value is right here, and it is > overwritten before we exit the non-error portion of the if statement. > I wonder if we couldn't save ourselves a few cycles and avoid an > unnecessary assignment by replacing the "!hsize" with a check for > "hsize <= 0" and just move the entire set of checks above down into > the lower block. > > > @@ -3901,6 +3899,9 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb, > > skb_release_head_state(nskb); > > __skb_push(nskb, doffset); > > } else { > > + if (hsize > len || !sg) > > + hsize = len; > > + > > Then you could essentially just add the "if (hsize < 0)" piece here as > an "else if" check and avoid the check if it isn't needed. Look correct, will post v2. Thanks! > > > nskb = __alloc_skb(hsize + doffset + headroom, > > GFP_ATOMIC, skb_alloc_rx_flag(head_skb), > > NUMA_NO_NODE); > > -- > > 2.1.0 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-01-12 7:21 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-11 12:45 [PATCH net-next 0/2] net: fix the features flag in sctp_gso_segment Xin Long 2021-01-11 12:45 ` [PATCH net-next 1/2] net: move the hsize check to the else block in skb_segment Xin Long 2021-01-11 12:45 ` [PATCH net-next 2/2] sctp: remove the NETIF_F_SG flag before calling skb_segment Xin Long 2021-01-11 16:26 ` [PATCH net-next 1/2] net: move the hsize check to the else block in skb_segment Alexander Duyck 2021-01-12 7:20 ` Xin Long
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).