linux-sctp.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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).