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

[Simon Horman <horms@kernel.org>]

On Mon, Jun 03, 2024 at 10:57:55AM +0800, D. Wythe wrote:
> 
> 
> 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?

Thanks, I agree with your reasoning.
And I think this is a good approach for this patch.

> 
> 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
> > ...
>