Re: [PATCH net-next 2/3] bpf: add TCP_BPF_RTO_MAX for bpf_setsockopt

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

On 2/13/25 10:56 PM, Jason Xing wrote:
> On Fri, Feb 14, 2025 at 2:40 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> On 2/13/25 10:12 PM, Jason Xing wrote:
>>> On Fri, Feb 14, 2025 at 1:41 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>>>
>>>> On 2/13/25 7:09 PM, Jason Xing wrote:
>>>>> On Fri, Feb 14, 2025 at 10:14 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>>>>>
>>>>>> On 2/13/25 3:57 PM, Jason Xing wrote:
>>>>>>> On Fri, Feb 14, 2025 at 7:41 AM Stanislav Fomichev<stfomichev@gmail.com> wrote:
>>>>>>>> On 02/13, Jason Xing wrote:
>>>>>>>>> Support bpf_setsockopt() to set the maximum value of RTO for
>>>>>>>>> BPF program.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Jason Xing<kerneljasonxing@gmail.com>
>>>>>>>>> ---
>>>>>>>>>      Documentation/networking/ip-sysctl.rst | 3 ++-
>>>>>>>>>      include/uapi/linux/bpf.h               | 2 ++
>>>>>>>>>      net/core/filter.c                      | 6 ++++++
>>>>>>>>>      tools/include/uapi/linux/bpf.h         | 2 ++
>>>>>>>>>      4 files changed, 12 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
>>>>>>>>> index 054561f8dcae..78eb0959438a 100644
>>>>>>>>> --- a/Documentation/networking/ip-sysctl.rst
>>>>>>>>> +++ b/Documentation/networking/ip-sysctl.rst
>>>>>>>>> @@ -1241,7 +1241,8 @@ tcp_rto_min_us - INTEGER
>>>>>>>>>
>>>>>>>>>      tcp_rto_max_ms - INTEGER
>>>>>>>>>           Maximal TCP retransmission timeout (in ms).
>>>>>>>>> -     Note that TCP_RTO_MAX_MS socket option has higher precedence.
>>>>>>>>> +     Note that TCP_BPF_RTO_MAX and TCP_RTO_MAX_MS socket option have the
>>>>>>>>> +     higher precedence for configuring this setting.
>>>>>>>> The cover letter needs more explanation about the motivation.
>>>>>>
>>>>>> +1
>>>>>>
>>>>>> I haven't looked at the patches. The cover letter has no word on the use case.
>>>>
>>>> The question was your _use case_ in bpf. Not what the TCP_RTO_MAX_MS does. Your
>>>> current use case is to have bpf setting it after reading the tcp header option,
>>>> like the selftest in patch 3?
>>>
>>> Oops, I misunderstood the real situation of the tcp header option
>>> test. My intention is to bpf_setsockopt() just like setget_sockopt
>>> does.
>>>
>>> Thanks for reminding me. I will totally remove the header test in the
>>> next version.
>>
>> If your use case was in the header, it is ok although it won't be the first
> 
> I was planning to add a simple test to only see if the rto max for bpf
> feature works, so I found the rto min selftests and then did a similar
> one.
> 
>> useful place I have in my mind. Regardless, it is useful to say a few words
>> where you are planning to set it in the bpf. During a cb in sockops or during
>> socket create ...etc. Without it, we can only guess from the selftest :(
> 
> I see your point. After evaluating and comparing those two tests, I
> think the setsock_opt is a better place to go. Do we even apply the
> use of rto min to setsock_opt as well?
> 
> What do you think?

Adding to sol_tcp_tests[] as Kuniyuki suggested should be the straight forward way.

Please still describe how you are going to use it in bpf in the cover letter.

> 
>>
>>>
>>>>
>>>>>
>>>>> I will add and copy some words from Eric's patch series :)
>>>>
>>>>
>>>>>>> I am targeting the net-next tree because of recent changes[1] made by
>>>>>>> Eric. It probably hasn't merged into the bpf-next tree.
>>>>>>
>>>>>> There is the bpf-next/net tree. It should have the needed changes.
>>>>>
>>>>> [1] was recently merged in the net-next tree, so the only one branch I
>>>>> can target is net-next.
>>>>>
>>>>> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=ae9b3c0e79bc
>>>>>
>>>>> Am I missing something?
>>>>
>>>> There is a net branch:
>>                 ^^^
>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git
>>>
>>> But this branch hasn't included the rto max feature. I was trying to
>>
>> Which branch? I was talking about the **net** branch. Not the master branch. Try
>> to pull again if your local copy does not have it. The net branch should have
>> the TCP_RTO_MAX_MS patches.
> 
> Oh, I always use the master branch, never heard of net branch. You're
> right, I checked out the net branch and then found it. Thanks.
> 
> One more thing I have to ask in advance is that in this case what the
> title looks like? [patch bpf] or [patch bpf net]?

[PATCH bpf-next]