Re: [PATCH bpf-next v4 2/3] net/smc: bpf: Introduce generic hook for handshake flow

[Martin KaFai Lau <martin.lau@linux.dev>]

On 11/6/25 12:34 AM, D. Wythe wrote:
> On Wed, Nov 05, 2025 at 08:16:45PM -0800, Martin KaFai Lau wrote:
>>
>>
>> On 11/5/25 6:33 PM, D. Wythe wrote:
>>> On Wed, Nov 05, 2025 at 02:58:48PM -0800, Martin KaFai Lau wrote:
>>>>
>>>>
>>>> On 11/4/25 11:01 PM, D. Wythe wrote:
>>>>> On Tue, Nov 04, 2025 at 04:03:46PM -0800, Martin KaFai Lau wrote:
>>>>>>
>>>>>>
>>>>>> On 11/2/25 11:31 PM, D. Wythe wrote:
>>>>>>> +#if IS_ENABLED(CONFIG_SMC_HS_CTRL_BPF)
>>>>>>> +#define smc_call_hsbpf(init_val, sk, func, ...) ({		\
>>>>>>> +	typeof(init_val) __ret = (init_val);			\
>>>>>>> +	struct smc_hs_ctrl *ctrl;				\
>>>>>>> +	rcu_read_lock();					\
>>>>>>> +	ctrl = rcu_dereference(sock_net(sk)->smc.hs_ctrl);	\
>>>>>>
>>>>>> The smc_hs_ctrl (and its ops) is called from the netns, so the
>>>>>> bpf_struct_ops is attached to a netns. Attaching bpf_struct_ops to a
>>>>>> netns has not been done before. More on this later.
>>>>>>
>>>>>>> +	if (ctrl && ctrl->func)					\
>>>>>>> +		__ret = ctrl->func(__VA_ARGS__);		\
>>>>>>> +
>>>>>>> +	if (static_branch_unlikely(&tcp_have_smc) && tp->syn_smc) {
>>>>>>> +		tp->syn_smc = !!smc_call_hsbpf(1, sk, syn_option, tp);
>>>>>>
>>>>>> ... so just pass tp instead of passing both sk and tp?
>>>>>>
>>>>>> [ ... ]
>>>>>>
>>>>>
>>>>> You're right, it is a bit redundant. However, if we merge the parameters,
>>>>> every user of this macro will be forced to pass tp. In fact, we’re
>>>>> already considering adding some callback functions that don’t take tp as
>>>>> a parameter.
>>>>
>>>> If the struct_ops callback does not take tp, then don't pass it to the
>>>> callback. I have a hard time to imagine why the bpf prog will not be
>>>> interested in the tp/sk pointer though.
>>>>
>>>> or you meant the caller does not have tp? and where is the future caller?
>>>
>>> My initial concern was that certain ctrl->func callbacks might
>>> eventually need to operate on an smc_sock rather than a tcp_sock.
>>
>> hmm...in that case, I think it first needs to understand where else
>> the smc struct_ops is planned to be called in the future. I thought
>> the smc struct_ops is something unique to the af_smc address family
>> but I suspect the future ops addition may not be the case. Can you
>> share some details on where the future callback will be? e.g. in
>> smc_{connect, sendmsg, recvmsg...} that has the smc_sock?
> 
> The design scope of hs_ctrl (handshake control) is limited to
> the SMC protocol's handshake phase. This means it will not be involved
> in data transmission functions like smc_sendmsg and smc_recvmsg, Instead,
> its focus is on:
> 
> 1. During the TCP three-way handshake
> 2. During the SMC protocol's own handshake. (proposal -> confirm ->
> accept)
> 
> Within the SMC module, hs_ctrl's primary future call points are
> concentrated within the __smc_connect() and smc_listen_work(). These
> two functions cover the SMC protocol handshake process.
> 
> And we have a plan involving private extensions to the SMC protocol.
> In the SMC protocol, different implementers can extend protocol functionality
> based on their Vendor Organizationally Unique Identifier (vendor_oui). You might
> notice that currently, the SMC implementation only has this vendor_oui field,
> but without corresponding functionality. This is highly significant for our
> applications, as many of our internal features rely on these private extensions.
> However, due to their inherent nature, these private features cannot be
> upstreamed. Therefore, BPF is the best way to implement these. Since
> these private extensions are essentially part of the SMC handshake
> process, hs_ctrl has become our first choice.
> 
> Beyond that, we are also considering other minor extensions to be
> implemented via hs_ctrl. These include assisting in the selection of the
> appropriate SMC device type and making decisions regarding which RDMA
> GID to use. (also in __smc_connect() and smc_listen_work()).

Thanks for the details.

Regarding the "net" passing and the future smc_sock, the net can still 
be obtained from smc_sock. It seems like a naming change on "tp" is 
needed when it may be a smc_sock in the future. It is a nit, so I will 
leave it as a fruit of thought for you and feel free to ignore.

Please re-spin.