| Message ID | cover.1610368918.git.lucien.xin@gmail.com |
|---|---|
| Headers | show |
| Series | net: fix the features flag in sctp_gso_segment | expand |
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 >
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 > >