[PATCH net-next] sctp: define sctp_packet_gso_append to build GSO frames

[PATCH net-next] sctp: define sctp_packet_gso_append to build GSO frames

[Xin Long]

Now sctp GSO uses skb_gro_receive() to append the data into head
skb frag_list. However it actually only needs very few code from
skb_gro_receive(). Besides, NAPI_GRO_CB has to be set while most
of its members are not needed here.

This patch is to add sctp_packet_gso_append() to build GSO frames
instead of skb_gro_receive(), and it would avoid many unnecessary
checks and make the code clearer.

Note that sctp will use page frags instead of frag_list to build
GSO frames in another patch. But it may take time, as sctp's GSO
frames may have different size. skb_segment() can only split it
into the frags with the same size, which would break the border
of sctp chunks.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/sctp/structs.h |  5 +++++
 net/sctp/output.c          | 28 ++++++++++++++++++----------
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index ebf809e..dbe1b91 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1133,6 +1133,11 @@ struct sctp_input_cb {
 };
 #define SCTP_INPUT_CB(__skb)	((struct sctp_input_cb *)&((__skb)->cb[0]))
 
+struct sctp_output_cb {
+	struct sk_buff *last;
+};
+#define SCTP_OUTPUT_CB(__skb)	((struct sctp_output_cb *)&((__skb)->cb[0]))
+
 static inline const struct sk_buff *sctp_gso_headskb(const struct sk_buff *skb)
 {
 	const struct sctp_chunk *chunk = SCTP_INPUT_CB(skb)->chunk;
diff --git a/net/sctp/output.c b/net/sctp/output.c
index e672dee..7f849b0 100644
--- a/net/sctp/output.c
+++ b/net/sctp/output.c
@@ -409,6 +409,21 @@ static void sctp_packet_set_owner_w(struct sk_buff *skb, struct sock *sk)
 	refcount_inc(&sk->sk_wmem_alloc);
 }
 
+static void sctp_packet_gso_append(struct sk_buff *head, struct sk_buff *skb)
+{
+	if (SCTP_OUTPUT_CB(head)->last == head)
+		skb_shinfo(head)->frag_list = skb;
+	else
+		SCTP_OUTPUT_CB(head)->last->next = skb;
+	SCTP_OUTPUT_CB(head)->last = skb;
+
+	head->truesize += skb->truesize;
+	head->data_len += skb->len;
+	head->len += skb->len;
+
+	__skb_header_release(skb);
+}
+
 static int sctp_packet_pack(struct sctp_packet *packet,
 			    struct sk_buff *head, int gso, gfp_t gfp)
 {
@@ -422,7 +437,7 @@ static int sctp_packet_pack(struct sctp_packet *packet,
 
 	if (gso) {
 		skb_shinfo(head)->gso_type = sk->sk_gso_type;
-		NAPI_GRO_CB(head)->last = head;
+		SCTP_OUTPUT_CB(head)->last = head;
 	} else {
 		nskb = head;
 		pkt_size = packet->size;
@@ -503,15 +518,8 @@ static int sctp_packet_pack(struct sctp_packet *packet,
 					 &packet->chunk_list);
 		}
 
-		if (gso) {
-			if (skb_gro_receive(&head, nskb)) {
-				kfree_skb(nskb);
-				return 0;
-			}
-			if (WARN_ON_ONCE(skb_shinfo(head)->gso_segs >=
-					 sk->sk_gso_max_segs))
-				return 0;
-		}
+		if (gso)
+			sctp_packet_gso_append(head, nskb);
 
 		pkt_count++;
 	} while (!list_empty(&packet->chunk_list));
-- 
2.1.0

Re: [PATCH net-next] sctp: define sctp_packet_gso_append to build GSO frames

[Marcelo Ricardo Leitner]

On Thu, Jun 14, 2018 at 07:37:02AM +0800, Xin Long wrote:
> Now sctp GSO uses skb_gro_receive() to append the data into head
> skb frag_list. However it actually only needs very few code from
> skb_gro_receive(). Besides, NAPI_GRO_CB has to be set while most
> of its members are not needed here.
> 
> This patch is to add sctp_packet_gso_append() to build GSO frames
> instead of skb_gro_receive(), and it would avoid many unnecessary
> checks and make the code clearer.
> 
> Note that sctp will use page frags instead of frag_list to build
> GSO frames in another patch. But it may take time, as sctp's GSO
> frames may have different size. skb_segment() can only split it
> into the frags with the same size, which would break the border
> of sctp chunks.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

I cannot test it, but it looks good to me. Thanks Xin

Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
>  include/net/sctp/structs.h |  5 +++++
>  net/sctp/output.c          | 28 ++++++++++++++++++----------
>  2 files changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index ebf809e..dbe1b91 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -1133,6 +1133,11 @@ struct sctp_input_cb {
>  };
>  #define SCTP_INPUT_CB(__skb)	((struct sctp_input_cb *)&((__skb)->cb[0]))
>  
> +struct sctp_output_cb {
> +	struct sk_buff *last;
> +};
> +#define SCTP_OUTPUT_CB(__skb)	((struct sctp_output_cb *)&((__skb)->cb[0]))
> +
>  static inline const struct sk_buff *sctp_gso_headskb(const struct sk_buff *skb)
>  {
>  	const struct sctp_chunk *chunk = SCTP_INPUT_CB(skb)->chunk;
> diff --git a/net/sctp/output.c b/net/sctp/output.c
> index e672dee..7f849b0 100644
> --- a/net/sctp/output.c
> +++ b/net/sctp/output.c
> @@ -409,6 +409,21 @@ static void sctp_packet_set_owner_w(struct sk_buff *skb, struct sock *sk)
>  	refcount_inc(&sk->sk_wmem_alloc);
>  }
>  
> +static void sctp_packet_gso_append(struct sk_buff *head, struct sk_buff *skb)
> +{
> +	if (SCTP_OUTPUT_CB(head)->last == head)
> +		skb_shinfo(head)->frag_list = skb;
> +	else
> +		SCTP_OUTPUT_CB(head)->last->next = skb;
> +	SCTP_OUTPUT_CB(head)->last = skb;
> +
> +	head->truesize += skb->truesize;
> +	head->data_len += skb->len;
> +	head->len += skb->len;
> +
> +	__skb_header_release(skb);
> +}
> +
>  static int sctp_packet_pack(struct sctp_packet *packet,
>  			    struct sk_buff *head, int gso, gfp_t gfp)
>  {
> @@ -422,7 +437,7 @@ static int sctp_packet_pack(struct sctp_packet *packet,
>  
>  	if (gso) {
>  		skb_shinfo(head)->gso_type = sk->sk_gso_type;
> -		NAPI_GRO_CB(head)->last = head;
> +		SCTP_OUTPUT_CB(head)->last = head;
>  	} else {
>  		nskb = head;
>  		pkt_size = packet->size;
> @@ -503,15 +518,8 @@ static int sctp_packet_pack(struct sctp_packet *packet,
>  					 &packet->chunk_list);
>  		}
>  
> -		if (gso) {
> -			if (skb_gro_receive(&head, nskb)) {
> -				kfree_skb(nskb);
> -				return 0;
> -			}
> -			if (WARN_ON_ONCE(skb_shinfo(head)->gso_segs >=
> -					 sk->sk_gso_max_segs))
> -				return 0;
> -		}
> +		if (gso)
> +			sctp_packet_gso_append(head, nskb);
>  
>  		pkt_count++;
>  	} while (!list_empty(&packet->chunk_list));
> -- 
> 2.1.0
>

Re: [PATCH net-next] sctp: define sctp_packet_gso_append to build GSO frames

[Neil Horman]

On Thu, Jun 14, 2018 at 07:37:02AM +0800, Xin Long wrote:
> Now sctp GSO uses skb_gro_receive() to append the data into head
> skb frag_list. However it actually only needs very few code from
> skb_gro_receive(). Besides, NAPI_GRO_CB has to be set while most
> of its members are not needed here.
> 
> This patch is to add sctp_packet_gso_append() to build GSO frames
> instead of skb_gro_receive(), and it would avoid many unnecessary
> checks and make the code clearer.
> 
> Note that sctp will use page frags instead of frag_list to build
> GSO frames in another patch. But it may take time, as sctp's GSO
> frames may have different size. skb_segment() can only split it
> into the frags with the same size, which would break the border
> of sctp chunks.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Do you have any performance numbers to compare with and without this patch?
Adding a function like this implies that any fixes that go into skb_gro_receive
now need to be evaluated for this function too, which means theres an implied
overhead in maintaining it.  If we're signing up for that, I'd like to know that
theres a significant performance benefit.

Neil

Re: [PATCH net-next] sctp: define sctp_packet_gso_append to build GSO frames

[Xin Long]

On Thu, Jun 14, 2018 at 8:46 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
> On Thu, Jun 14, 2018 at 07:37:02AM +0800, Xin Long wrote:
>> Now sctp GSO uses skb_gro_receive() to append the data into head
>> skb frag_list. However it actually only needs very few code from
>> skb_gro_receive(). Besides, NAPI_GRO_CB has to be set while most
>> of its members are not needed here.
>>
>> This patch is to add sctp_packet_gso_append() to build GSO frames
>> instead of skb_gro_receive(), and it would avoid many unnecessary
>> checks and make the code clearer.
>>
>> Note that sctp will use page frags instead of frag_list to build
>> GSO frames in another patch. But it may take time, as sctp's GSO
>> frames may have different size. skb_segment() can only split it
>> into the frags with the same size, which would break the border
>> of sctp chunks.
>>
>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> Do you have any performance numbers to compare with and without this patch?
> Adding a function like this implies that any fixes that go into skb_gro_receive
> now need to be evaluated for this function too, which means theres an implied
> overhead in maintaining it.  If we're signing up for that, I'd like to know that
> theres a significant performance benefit.
Hi Neil,

I don't think there's a noticeable performance benefit since it's
just avoided some checks and variables settings.

The new function makes SCTP GSO code clearer and readable,
as skb_gro_receive() should only be used in the GRO code paths,
it's confusing in sctp tx path.

We're doing this, actually because skb_gro_receive() is being
changed now, it would not be suitable for SCTP GSO, see:
https://www.spinics.net/lists/netdev/msg507716.html

And also, we believe page frags will be used to replace frag_list
to build SCTP GSO frames soon. After that, this function will also
be dropped.

Re: [PATCH net-next] sctp: define sctp_packet_gso_append to build GSO frames

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Wed, 13 Jun 2018 20:46:43 -0400

> Do you have any performance numbers to compare with and without this
> patch?  Adding a function like this implies that any fixes that go
> into skb_gro_receive now need to be evaluated for this function too,
> which means theres an implied overhead in maintaining it.  If we're
> signing up for that, I'd like to know that theres a significant
> performance benefit.

Neil, I asked Xin and Marcelo to do this.

There is no reason for GSO code to use a GRO helper.

And this is, in particular, blocking some skb_gro_receive() surgery
I plan to perform.

Re: [PATCH net-next] sctp: define sctp_packet_gso_append to build GSO frames

[Neil Horman]

On Thu, Jun 14, 2018 at 09:21:52AM +0800, Xin Long wrote:
> On Thu, Jun 14, 2018 at 8:46 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
> > On Thu, Jun 14, 2018 at 07:37:02AM +0800, Xin Long wrote:
> >> Now sctp GSO uses skb_gro_receive() to append the data into head
> >> skb frag_list. However it actually only needs very few code from
> >> skb_gro_receive(). Besides, NAPI_GRO_CB has to be set while most
> >> of its members are not needed here.
> >>
> >> This patch is to add sctp_packet_gso_append() to build GSO frames
> >> instead of skb_gro_receive(), and it would avoid many unnecessary
> >> checks and make the code clearer.
> >>
> >> Note that sctp will use page frags instead of frag_list to build
> >> GSO frames in another patch. But it may take time, as sctp's GSO
> >> frames may have different size. skb_segment() can only split it
> >> into the frags with the same size, which would break the border
> >> of sctp chunks.
> >>
> >> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > Do you have any performance numbers to compare with and without this patch?
> > Adding a function like this implies that any fixes that go into skb_gro_receive
> > now need to be evaluated for this function too, which means theres an implied
> > overhead in maintaining it.  If we're signing up for that, I'd like to know that
> > theres a significant performance benefit.
> Hi Neil,
> 
> I don't think there's a noticeable performance benefit since it's
> just avoided some checks and variables settings.
> 
> The new function makes SCTP GSO code clearer and readable,
> as skb_gro_receive() should only be used in the GRO code paths,
> it's confusing in sctp tx path.
> 
> We're doing this, actually because skb_gro_receive() is being
> changed now, it would not be suitable for SCTP GSO, see:
> https://www.spinics.net/lists/netdev/msg507716.html
> 
Ok, I'm not on board if the only reason was to improve readability, as I didn't
want to maintain two separate code paths, but if skb_gro_receive isn't going to
be useable, then I'm ok with it

Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH net-next] sctp: define sctp_packet_gso_append to build GSO frames

[Neil Horman]

On Wed, Jun 13, 2018 at 07:05:59PM -0700, David Miller wrote:
> From: Neil Horman <nhorman@tuxdriver.com>
> Date: Wed, 13 Jun 2018 20:46:43 -0400
> 
> > Do you have any performance numbers to compare with and without this
> > patch?  Adding a function like this implies that any fixes that go
> > into skb_gro_receive now need to be evaluated for this function too,
> > which means theres an implied overhead in maintaining it.  If we're
> > signing up for that, I'd like to know that theres a significant
> > performance benefit.
> 
> Neil, I asked Xin and Marcelo to do this.
> 
> There is no reason for GSO code to use a GRO helper.
> 
> And this is, in particular, blocking some skb_gro_receive() surgery
> I plan to perform.
> 
I agree, I wasn't aware of your intentions regarding skb_gro_receive, and have
acked the patch

Neil

Re: [PATCH net-next] sctp: define sctp_packet_gso_append to build GSO frames

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Thu, 14 Jun 2018 07:37:02 +0800

> Now sctp GSO uses skb_gro_receive() to append the data into head
> skb frag_list. However it actually only needs very few code from
> skb_gro_receive(). Besides, NAPI_GRO_CB has to be set while most
> of its members are not needed here.
> 
> This patch is to add sctp_packet_gso_append() to build GSO frames
> instead of skb_gro_receive(), and it would avoid many unnecessary
> checks and make the code clearer.
> 
> Note that sctp will use page frags instead of frag_list to build
> GSO frames in another patch. But it may take time, as sctp's GSO
> frames may have different size. skb_segment() can only split it
> into the frags with the same size, which would break the border
> of sctp chunks.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Applied to 'net', thanks Xin.