Re: [PATCH net-next v11 03/14] net: Add lease info to queue-get response

[Daniel Borkmann <daniel@iogearbox.net>]

On 4/9/26 3:43 PM, Daniel Borkmann wrote:
> On 4/9/26 12:12 AM, Jakub Kicinski wrote:
>> On Wed, 8 Apr 2026 11:09:34 +0200 Daniel Borkmann wrote:
>>>>> +void netif_put_rx_queue_lease_locked(struct net_device *orig_dev,
>>>>> +                     struct net_device *dev)
>>>>> +{
>>>>> +    if (orig_dev != dev)
>>>>> +        netdev_unlock(dev);
>>>>> +}
>>>>
>>>> Pretty sure I already complained about these ugly helpers.
>>>> I'll try to find the time tomorrow to come up with something better.
>>>
>>> Ok, sounds good. Happy to adapt if you find something better and then I'll
>>> work this into the series, and also integrate the things mentioned in my
>>> cover letter reply (netkit nl dump + additional tests).
>>
>> Hi! How would you feel about something like the following on top?
>>
>> --->8----------
>>
>> net: remove the netif_get_rx_queue_lease_locked() helpers
>>
>> The netif_get_rx_queue_lease_locked() API hides the locking
>> and the descend onto the leased queue. Making the code
>> harder to follow (at least to me). Remove the API and open
>> code the descend a bit. Most of the code now looks like:
>>
>>   if (!leased)
>>       return __helper(x);
>>
>>   hw_rxq = ..
>>   netdev_lock(hw_rxq->dev);
>>   ret = __helper(x);
>>   netdev_unlock(hw_rxq->dev);
>>
>>   return ret;
>>
>> Of course if we have more code paths that need the wrapping
>> we may need to revisit. For now, IMHO, having to know what
>> netif_get_rx_queue_lease_locked() does is not worth the 20LoC
>> it saves.
>>
>> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> Thanks for looking into it! That looks good to me. I've also retested that
> it still works.
> 
> Maybe small nits could be below to move the netif_is_queue_leasee into the
> netdev_rx_queue.h header since its used outside of core and it might be
> worth to also have the lock assertion in netdev_queue_get_dma_dev.
> 
> Do you want me to add your patch on top for a v12 of the series?
> 
> Thanks,
> Daniel
> 
>   include/net/netdev_rx_queue.h | 2 ++
>   net/core/dev.h                | 1 -
>   net/core/netdev_queues.c      | 2 ++
>   net/xdp/xsk.c                 | 2 --
>   4 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/netdev_rx_queue.h b/include/net/netdev_rx_queue.h
> index 9415a94d333d..e9f7f098609b 100644
> --- a/include/net/netdev_rx_queue.h
> +++ b/include/net/netdev_rx_queue.h
> @@ -76,6 +76,8 @@ struct netdev_rx_queue *
>   __netif_get_rx_queue_lease(struct net_device **dev, unsigned int *rxq,
>                  enum netif_lease_dir dir);
> 
> +bool netif_is_queue_leasee(const struct net_device *dev);
> +
>   int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq);
>   void netdev_rx_queue_lease(struct netdev_rx_queue *rxq_dst,
>                  struct netdev_rx_queue *rxq_src);
> diff --git a/net/core/dev.h b/net/core/dev.h
> index 376bac4a82da..95edb2d4eff8 100644
> --- a/net/core/dev.h
> +++ b/net/core/dev.h
> @@ -101,7 +101,6 @@ int netdev_queue_config_validate(struct net_device *dev, int rxq_idx,
> 
>   bool netif_rxq_has_mp(struct net_device *dev, unsigned int rxq_idx);
>   bool netif_rxq_is_leased(struct net_device *dev, unsigned int rxq_idx);
> -bool netif_is_queue_leasee(const struct net_device *dev);

(Ok, if so then netif_rxq_is_leased would have to move too..
  maybe its fine as-is then.)

Thanks,
Daniel