From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Abeni Subject: Re: [PATCH RFC 1/3] net: Prepare GSO return values for fraglist GSO. Date: Tue, 08 Jan 2019 14:53:52 +0100 Message-ID: <7f353a949e6d8bd89a0b7b63e07cbcf3cf66b6a9.camel@redhat.com> References: <20181221075334.9000-1-steffen.klassert@secunet.com> <20181221075334.9000-2-steffen.klassert@secunet.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: Willem de Bruijn , "Jason A. Donenfeld" To: Steffen Klassert , netdev@vger.kernel.org Return-path: Received: from mx1.redhat.com ([209.132.183.28]:55938 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728551AbfAHNxz (ORCPT ); Tue, 8 Jan 2019 08:53:55 -0500 In-Reply-To: <20181221075334.9000-2-steffen.klassert@secunet.com> Sender: netdev-owner@vger.kernel.org List-ID: On Fri, 2018-12-21 at 08:53 +0100, Steffen Klassert wrote: > On fraglist GSO, we don't need to clone the original > skb. So we don't have anything to return to free. > Prepare GSO that it frees the original skb only > if the return pointer really changed. Fraglist > GSO frees the original skb itself on error and > returns -EREMOTE in this case. I think it would be nicer preseving the same sematic with gro list, so that we don't have to add this special handling. e.g. calling skb_get(skb) in skb_segment_list() when successful, would avoid the special handling for the no error case (at the cost of 2 atomic ops per gso_list packet) > diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c > index 4ae87c5ce2e3..1941dc2a80a0 100644 > --- a/net/xfrm/xfrm_output.c > +++ b/net/xfrm/xfrm_output.c > @@ -183,7 +183,8 @@ static int xfrm_output_gso(struct net *net, struct sock *sk, struct sk_buff *skb > BUILD_BUG_ON(sizeof(*IPCB(skb)) > SKB_SGO_CB_OFFSET); > BUILD_BUG_ON(sizeof(*IP6CB(skb)) > SKB_SGO_CB_OFFSET); > segs = skb_gso_segment(skb, 0); > - kfree_skb(skb); > + if (segs != skb) > + kfree_skb(skb); > if (IS_ERR(segs)) what if IS_ERR(segs) == -EREMOTE here? > return PTR_ERR(segs); > if (segs == NULL)