Message-ID: <39fbfcb8-20a4-4693-af24-ea59a726bbec@yahoo.com>
Date: Mon, 17 Nov 2025 12:31:00 +0100
From: Marek Mietus <mmietus97@...oo.com>
To: Sabrina Dubroca <sd@...asysnail.net>, Eric Dumazet <edumazet@...gle.com>
Cc: netdev@...r.kernel.org, kuba@...nel.org, pabeni@...hat.com,
 davem@...emloft.net
Subject: Re: [PATCH net-next v4 02/14] net: skb: use dstref for storing dst
 entry

Dear Eric and Sabrina, I'm replying to both of you here.

W dniu 11/13/25 o 19:17, Sabrina Dubroca pisze:
> Eric, it seems your email didn't make it to netdev, quoting:
> 
> 2025-11-13, 02:38:02 -0800, Eric Dumazet wrote:
>> On Thu, Nov 13, 2025 at 1:37 AM Marek Mietus <mmietus97@...oo.com> wrote:
>>
>>> W dniu 11/12/25 o 18:09, Sabrina Dubroca pisze:
>>>> 2025-11-12, 08:27:08 +0100, Marek Mietus wrote:
>>>>> Use the newly introduced dstref object for storing the dst entry
>>>>> in skb instead of using _skb_refdst, and remove code related
>>>>> to _skb_refdst.
>>>>
>>>> This is an important change to a very core part of networking. You
>>>> need to CC all the networking maintainers/reviewers for this series
>>>> (ask scripts/get_maintainer.pl).
>>>
>>> Noted for next time.
>>>
>>>>
>>>>> This is mostly a cosmetic improvement. It improves readability
>>>>
>>>> That rename, and the rest of the changes in this series. is causing
>>>> some non-negligible churn and will take a while to review, to ensure
>>>> all the conversions are correct.
>>>>
>>>> @Maintainers can I get some time to look at this in detail?
>>>>
>>>
>>> I figured it would require a thorough review.
>>> Thank you for taking the time to look at it!
>>>
>>>>
>>>> Also, I'm not sure how we ended up from the previous proposal ("some
>>>> tunnels are under RCU so they don't need a reference" [1]) to this.
>>>>
>>>> [1]
>>> https://lore.kernel.org/netdev/20250922110622.10368-1-mmietus97@yahoo.com/
>>>>
>>>
>>> As previously discussed with Jakub [2], tunnels that use
>>> udp_tunnel_dst_lookup
>>> add notable complexity because the returned dst could either be from
>>> ip_route_output_key (referenced) or from the dst_cache (which I'm changing
>>> to
>>> be noref). There are also other tunnels that follow a similar pattern.
> 
> But IMO Jakub's comment about technical debt is not addressed by
> pushing dstref all over the tunnel code.
> 
> 

I understood it differently. I thought the aforementioned debt referred
to maintaining two different APIs that accomplish the same result, which
is why I took the time to replace all callers in all of the tunnels to use
the new API, and removed the old API. Maybe Jakub can clarify.

>>> The cleanest way to keep track of which dst is referenced and which isn't
>>> is to borrow existing refdst concepts. This allows us to more easily track
>>> the ref state of dst_entries in later flows to avoid unnecessarily taking
>>> a reference. I played around with a couple implementations and this turned
>>> out to be the most elegant. It's a big change, but it's mostly semantic.
>>>
>>> [2] https://lore.kernel.org/netdev/20250923184856.6cce6530@kernel.org/
>>
>>
>> I have not seen the series, so I had to go to the archives.
>>
>> Too much code churn for my taste, and a true nightmare for future backports
>> to stable kernels.
>>
>> Unless I am mistaken, this is your first submission to the linux kernel,
>> please start with more manageable patches.
> 

You are correct. This is my first submission. Initially, this series was much
smaller, but that changed due to the generous feedback from Sabrina, Jakub and
Paolo. I tried to make this series smaller, but that ended up in code duplication
between tunnels and some spaghetti code. I did spend quite some time on this new
version, so I'd love to hear your suggestions / feedback if you have any, as I'd
like to continue working on this feature.

Powered by blists - more mailing lists