Re: [PATCH bpf-next v8 1/4] bpf: Add update_socket_protocol hook

From: Matthieu Baerts <matthieu.baerts@tessares.net>
To: Geliang Tang <geliang.tang@suse.com>
Cc: MPTCP Upstream <mptcp@lists.linux.dev>
Subject: Re: [PATCH bpf-next v8 1/4] bpf: Add update_socket_protocol hook
Date: Thu, 3 Aug 2023 15:18:12 +0200	[thread overview]
Message-ID: <4eec4a85-c055-473f-918d-754c0ae21b45@tessares.net> (raw)
In-Reply-To: <20230803130540.GA2792@bogon>

Hi Geliang,

(only with MPTCP list because there are too many people involved and
that's just a detail)

On 03/08/2023 15:05, Geliang Tang wrote:
> On Thu, Aug 03, 2023 at 02:53:38PM +0200, Simon Horman wrote:
>> On Thu, Aug 03, 2023 at 03:30:39PM +0800, Geliang Tang wrote:
>>> Add a hook named update_socket_protocol in __sys_socket(), for bpf
>>> progs to attach to and update socket protocol. One user case is to
>>> force legacy TCP apps to create and use MPTCP sockets instead of
>>> TCP ones.
>>>
>>> Define a mod_ret set named bpf_mptcp_fmodret_ids, add the hook
>>> update_socket_protocol into this set, and register it in
>>> bpf_mptcp_kfunc_init().
>>>
>>> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
>>
>> ...
>>
>>> diff --git a/net/socket.c b/net/socket.c
>>> index 2b0e54b2405c..586a437d7a5e 100644
>>> --- a/net/socket.c
>>> +++ b/net/socket.c
>>> @@ -1644,11 +1644,36 @@ struct file *__sys_socket_file(int family, int type, int protocol)
>>>  	return sock_alloc_file(sock, flags, NULL);
>>>  }
>>>  
>>> +/**
>>
>> Hi Geliang Tang,
>>
>> nit: The format of the text below is not in kernel doc format,
>>      so it is probably better if the comment begins with '/*'
>>      rather than '/**'.
> 
> I do use '/*' here first, but got a checkpatch.pl warning:
> 
>   ./scripts/checkpatch.pl v8-0001-bpf-Add-update_socket_protocol-hook.patch 
>   WARNING: networking block comments don't use an empty /* line, use /* Comment...
>   #63: FILE: net/socket.c:1648:
>   +/*
>   + *	A hook for bpf progs to attach to and update socket protocol.
> 
>   total: 0 errors, 1 warnings, 0 checks, 59 lines checked
> 
> And I found that other comments in net/socket.c all begins with '/**'.
> So I use '/**' here too.

From what I understand, when using a comment block starting with "/**",
it is to insert kernel doc format (similar to doxygen if you know about
it) where you can generally see the description of a function with
parameters starting with "@", etc. e.g. what is done above
"sock_recvmsg()" in "net/socket.c".

But I see indeed there are many other "random" comments starting with
"/**" but I guess that's because "net/socket.c" is very old, when
checkpatch.pl didn't exist.

I think the best is follow the "new" style, e.g. what is done above
"skb_is_swtx_tstamp()" in "net/socket.c", starting on the first line and
without tabulations after the "*", e.g.

  /* A hook for bpf progs to attach to and update socket protocol.
   *
   * A static noinline (...)
   * (...)
   */

WDYT?
Would it fix 'checkpatch.pl' warnings?

(if you send a new version, please add the Closes tag in this patch as
well if you don't mind. You can also keep my Reviewed-by and Acked-by
tags if you want but then put your Signed-off-by tag at the end: the
last tag should be a Signed-off-by tag of the person sending the
patches, you in this case :) )

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/79

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net