Re: [PATCH net-next v4 3/3] net/smc: Introduce IPPROTO_SMC

["D. Wythe" <alibuda@linux.alibaba.com>]

On 6/1/24 9:06 PM, Simon Horman wrote:
> On Wed, May 29, 2024 at 11:59:07AM +0800, D. Wythe wrote:
>> From: "D. Wythe" <alibuda@linux.alibaba.com>
>>
>> This patch allows to create smc socket via AF_INET,
>> similar to the following code,
>>
>> /* create v4 smc sock */
>> v4 = socket(AF_INET, SOCK_STREAM, IPPROTO_SMC);
>>
>> /* create v6 smc sock */
>> v6 = socket(AF_INET6, SOCK_STREAM, IPPROTO_SMC);
>>
>> There are several reasons why we believe it is appropriate here:
>>
>> 1. For smc sockets, it actually use IPv4 (AF-INET) or IPv6 (AF-INET6)
>> address. There is no AF_SMC address at all.
>>
>> 2. Create smc socket in the AF_INET(6) path, which allows us to reuse
>> the infrastructure of AF_INET(6) path, such as common ebpf hooks.
>> Otherwise, smc have to implement it again in AF_SMC path.
>>
>> Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
> ...
>
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> ...
>
>> @@ -3594,9 +3595,31 @@ static int __init smc_init(void)
>>   		goto out_lo;
>>   	}
>>   
>> +	rc = proto_register(&smc_inet_prot, 1);
>> +	if (rc) {
>> +		pr_err("%s: proto_register smc_inet_prot fails with %d\n", __func__, rc);
> Hi,
>
> FWIIW, my feeling is that if a log message includes __func__ then it should
> be a debug level message, and even then I'm dubious about the value of
> __func__: we do have many tools including dynamic tracing or pinpointing
> problems.
>
> So I would suggest rephrasing this message and dropping __func__.
> Or maybe removing it entirely.
> Or if not, lowering the priority of this message to debug.
>
> If for some reason __func__ remains, please do consider wrapping
> the line to 80c columns or less, which can be trivially done here
> (please don't split the format string in any case).
>
> Flagged by checkpatch.pl --max-line-length=80


Hi Simon,

Thank you very much for your feedback.

Allow me to briefly explain the reasons for using pr_err and __func__ here.

Regarding pr_err, the failure here leads to the failure of the module 
loading, which is definitely an error-level message rather than a 
debug-level one.

As for __func__, I must admit that the purpose here is simply to align 
with the format of other error messages in smc_init(). In fact, I also 
feel that the presence of
__func__ doesn't hold significant value because this error will only 
occur within this function. It's meaningless information for both users 
and kernel developers.
Perhaps a more suitable format would be “smc: xxx: %d”.

However, if changes are needed, I think they should be made across the 
board in order to maintain a consistent style. Maybe this can be 
addressed by
submitting a new patch after this patch. @Wenjia, what do you think?

Therefore, for now, I would like to wrap this line to not exceed 80 
characters, to ensure it can pass the checkpatch.pl.
What do you think?

Best wishes,
D. Wythe

>
>> +		goto out_ulp;
>> +	}
>> +	inet_register_protosw(&smc_inet_protosw);
>> +#if IS_ENABLED(CONFIG_IPV6)
>> +	rc = proto_register(&smc_inet6_prot, 1);
>> +	if (rc) {
>> +		pr_err("%s: proto_register smc_inet6_prot fails with %d\n", __func__, rc);
> Here too.
>
>> +		goto out_inet_prot;
>> +	}
>> +	inet6_register_protosw(&smc_inet6_protosw);
>> +#endif
> ...