Message-ID: <CAL+tcoAhQTMBxC=qZO0NpiqRCdfGEkD7iWxSg7Odfs4eO7N_JQ@mail.gmail.com>
Date: Sat, 15 Feb 2025 07:53:53 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>
Cc: Stanislav Fomichev <stfomichev@...il.com>, davem@...emloft.net, edumazet@...gle.com, 
	kuba@...nel.org, pabeni@...hat.com, dsahern@...nel.org, ast@...nel.org, 
	daniel@...earbox.net, andrii@...nel.org, eddyz87@...il.com, song@...nel.org, 
	yonghong.song@...ux.dev, john.fastabend@...il.com, kpsingh@...nel.org, 
	sdf@...ichev.me, haoluo@...gle.com, jolsa@...nel.org, horms@...nel.org, 
	ncardwell@...gle.com, kuniyu@...zon.com, bpf@...r.kernel.org, 
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/3] bpf: add TCP_BPF_RTO_MAX for bpf_setsockopt

On Sat, Feb 15, 2025 at 7:45 AM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>
> On 2/13/25 10:56 PM, Jason Xing wrote:
> > On Fri, Feb 14, 2025 at 2:40 PM Martin KaFai Lau <martin.lau@...ux.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@...ux.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@...ux.dev> wrote:
> >>>>>>
> >>>>>> On 2/13/25 3:57 PM, Jason Xing wrote:
> >>>>>>> On Fri, Feb 14, 2025 at 7:41 AM Stanislav Fomichev<stfomichev@...il.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@...il.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.

Sure, I will.

Another related topic about rto min test, do you think it's necessary
to add TCP_BPF_RTO_MIN into the setget_sockopt test?

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

Thanks. Will do it.

Thanks,
Jason

Powered by blists - more mailing lists