Re: [PATCH bpf-next 2/5] net/smc: allow smc to negotiate protocols on policies

[Kui-Feng Lee <sinquersw@gmail.com>]

On 4/26/23 20:30, D. Wythe wrote:
> 
> Hi Lee,
> 
> 
> On 4/27/23 12:47 AM, Kui-Feng Lee wrote:
>>
>>
>> On 4/26/23 02:24, D. Wythe wrote:
>>> From: "D. Wythe" <alibuda@linux.alibaba.com>
>>> diff --git a/net/smc/bpf_smc.c b/net/smc/bpf_smc.c
>>> new file mode 100644
>>> index 0000000..0c0ec05
>>> --- /dev/null
>>> +++ b/net/smc/bpf_smc.c
>>> @@ -0,0 +1,201 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>> ... cut ...
> 
> Will fix it, Thanks.
> 
>>> +
>>> +/* register ops */
>>> +int smc_sock_register_negotiator_ops(struct smc_sock_negotiator_ops 
>>> *ops)
>>> +{
>>> +    int ret;
>>> +
>>> +    ret = smc_sock_validate_negotiator_ops(ops);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    /* calt key by name hash */
>>> +    ops->key = jhash(ops->name, sizeof(ops->name), strlen(ops->name));
>>> +
>>> +    spin_lock(&smc_sock_negotiator_list_lock);
>>> +    if (smc_negotiator_ops_get_by_key(ops->key)) {
>>> +        pr_notice("smc: %s negotiator already registered\n", 
>>> ops->name);
>>> +        ret = -EEXIST;
>>> +    } else {
>>> +        list_add_tail_rcu(&ops->list, &smc_sock_negotiator_list);
>>> +    }
>>> +    spin_unlock(&smc_sock_negotiator_list_lock);
>>> +    return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(smc_sock_register_negotiator_ops);
>>
>> This and following functions are not specific to BPF, right?
>> I found you have more BPF specific code in this file in following
>> patches.  But, I feel these function should not in this file since
>> they are not BPF specific because file name "bpf_smc.c" hints.
> 
> Yes. Logically those functions are not suitable for being placed in 
> "bpf_smc.c".
> However, since SMC is compiled as modules by default, and currently
> struct ops needs to be built in, or specific symbols will not be found 
> during linking.
> 
> Of course, I can separate those this function in another new file, which 
> can also be built in.
> I may have to introduce a new KConfig likes SMC_NEGOTIATOR. But this 
> feature is  only effective
> when eBPF exists, so from the perspective of SMC, it would also be kind 
> of weird.
On the other hand, this feature is only effective when SMC exists.
Even without BPF, you still can implement a negotiator in a module.
Since you have exported these symbols, I suspect that you expect
negotiators in modules or builtin, right?  If I am wrong about exports,
perhaps you should stop exporting since they are used locally only.

> 
> But whatever, if you do think it's necessary, I can split it into two 
> files.
> 
> Besh wishes.
> D. Wythe
> 
> 
>