diff mbox series

[03/12,v2,RFC] skbuff: simplify sock_zerocopy_put

Message ID 20201222000926.1054993-4-jonathan.lemon@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Generic zcopy_* functions | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Guessed tree name to be net-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 12 maintainers not CCed: gnault@redhat.com davem@davemloft.net viro@zeniv.linux.org.uk willemb@google.com bsd@fb.com pabeni@redhat.com linmiaohe@huawei.com kuba@kernel.org elver@google.com linyunsheng@huawei.com kyk.segfault@gmail.com steffen.klassert@secunet.com
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 1 this patch: 1
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: From:/Signed-off-by: email address mismatch: 'From: Jonathan Lemon <bsd@fb.com>' != 'Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>'
netdev/build_allmodconfig_warn success Errors and warnings before: 1 this patch: 1
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Jonathan Lemon Dec. 22, 2020, 12:09 a.m. UTC 
From: Jonathan Lemon <bsd@fb.com>

All 'struct ubuf_info' users should have a callback defined.
Remove the dead code path to consume_skb(), which makes
unwarranted assumptions about how the structure was allocated.

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 net/core/skbuff.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Comments

Willem de Bruijn Dec. 22, 2020, 2:43 p.m. UTC | #1 
On Mon, Dec 21, 2020 at 7:09 PM Jonathan Lemon <jonathan.lemon@gmail.com> wrote:
>
> From: Jonathan Lemon <bsd@fb.com>
>
> All 'struct ubuf_info' users should have a callback defined.
> Remove the dead code path to consume_skb(), which makes
> unwarranted assumptions about how the structure was allocated.
>
> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>

Please link to the commit I shared that made the consume_skb path
obsolete. Before that this would have been unsafe.

should have a callback defined -> have a callback defined as of commit
0a4a060bb204 ("sock: fix
zerocopy_success regression with msg_zerocopy").

With that explanation why this is correct

Acked-by: Willem de Bruijn <willemb@google.com>
David Ahern Dec. 22, 2020, 4:52 p.m. UTC | #2 
On 12/21/20 5:09 PM, Jonathan Lemon wrote:
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 327ee8938f78..ea32b3414ad6 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1245,12 +1245,8 @@ EXPORT_SYMBOL_GPL(sock_zerocopy_callback);
>  
>  void sock_zerocopy_put(struct ubuf_info *uarg)
>  {
> -	if (uarg && refcount_dec_and_test(&uarg->refcnt)) {
> -		if (uarg->callback)
> -			uarg->callback(uarg, uarg->zerocopy);
> -		else
> -			consume_skb(skb_from_uarg(uarg));
> -	}
> +	if (uarg && refcount_dec_and_test(&uarg->refcnt))
> +		uarg->callback(uarg, uarg->zerocopy);
>  }
>  EXPORT_SYMBOL_GPL(sock_zerocopy_put);
>  
> 

since it is down to 2 lines, move to skbuff.h as an inline?
David Ahern Dec. 22, 2020, 4:56 p.m. UTC | #3 
On 12/22/20 9:52 AM, David Ahern wrote:
> On 12/21/20 5:09 PM, Jonathan Lemon wrote:
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index 327ee8938f78..ea32b3414ad6 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -1245,12 +1245,8 @@ EXPORT_SYMBOL_GPL(sock_zerocopy_callback);
>>  
>>  void sock_zerocopy_put(struct ubuf_info *uarg)
>>  {
>> -	if (uarg && refcount_dec_and_test(&uarg->refcnt)) {
>> -		if (uarg->callback)
>> -			uarg->callback(uarg, uarg->zerocopy);
>> -		else
>> -			consume_skb(skb_from_uarg(uarg));
>> -	}
>> +	if (uarg && refcount_dec_and_test(&uarg->refcnt))
>> +		uarg->callback(uarg, uarg->zerocopy);
>>  }
>>  EXPORT_SYMBOL_GPL(sock_zerocopy_put);
>>  
>>
> 
> since it is down to 2 lines, move to skbuff.h as an inline?
> 

nm. that is done in patch 5.
diff mbox series

Patch

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 327ee8938f78..ea32b3414ad6 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1245,12 +1245,8 @@  EXPORT_SYMBOL_GPL(sock_zerocopy_callback);
 
 void sock_zerocopy_put(struct ubuf_info *uarg)
 {
-	if (uarg && refcount_dec_and_test(&uarg->refcnt)) {
-		if (uarg->callback)
-			uarg->callback(uarg, uarg->zerocopy);
-		else
-			consume_skb(skb_from_uarg(uarg));
-	}
+	if (uarg && refcount_dec_and_test(&uarg->refcnt))
+		uarg->callback(uarg, uarg->zerocopy);
 }
 EXPORT_SYMBOL_GPL(sock_zerocopy_put);