Message-ID: <73a104e0-d73f-4836-92fd-4bef415900d4@kernel.org>
Date: Thu, 12 Sep 2024 17:26:09 +0200
From: Matthieu Baerts <matttbe@...nel.org>
To: Mina Almasry <almasrymina@...gle.com>
Cc: mptcp@...ts.linux.dev, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Kaiyuan Zhang <kaiyuanz@...gle.com>,
 Willem de Bruijn <willemb@...gle.com>,
 Pavel Begunkov <asml.silence@...il.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] memory-provider: fix compilation issue without
 SYSFS

Hi Mina,

Thank you for your reply!

On 12/09/2024 14:49, Mina Almasry wrote:
> On Thu, Sep 12, 2024 at 3:25 AM Matthieu Baerts (NGI0)
> <matttbe@...nel.org> wrote:
>>
>> When CONFIG_SYSFS is not set, the kernel fails to compile:
>>
>>      net/core/page_pool_user.c:368:45: error: implicit declaration of function 'get_netdev_rx_queue_index' [-Werror=implicit-function-declaration]
>>       368 |                 if (pool->slow.queue_idx == get_netdev_rx_queue_index(rxq)) {
>>           |                                             ^~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> When CONFIG_SYSFS is not set, get_netdev_rx_queue_index() is not defined
>> as well. In this case, page_pool_check_memory_provider() cannot check
>> the memory provider, and a success answer can be returned instead.
>>
> 
> Thanks Matthieu, and sorry about that.
> 
> I have reproduced the build error and the fix resolves it. But...
> 
>> Fixes: 0f9214046893 ("memory-provider: dmabuf devmem memory provider")
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@...nel.org>
>> ---
>>  net/core/page_pool_user.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/net/core/page_pool_user.c b/net/core/page_pool_user.c
>> index 48335766c1bf..a98c0a76b33f 100644
>> --- a/net/core/page_pool_user.c
>> +++ b/net/core/page_pool_user.c
>> @@ -353,6 +353,7 @@ void page_pool_unlist(struct page_pool *pool)
>>  int page_pool_check_memory_provider(struct net_device *dev,
>>                                     struct netdev_rx_queue *rxq)
>>  {
>> +#ifdef CONFIG_SYSFS
>>         struct net_devmem_dmabuf_binding *binding = rxq->mp_params.mp_priv;
>>         struct page_pool *pool;
>>         struct hlist_node *n;
>> @@ -372,6 +373,9 @@ int page_pool_check_memory_provider(struct net_device *dev,
>>         }
>>         mutex_unlock(&page_pools_lock);
>>         return -ENODATA;
>> +#else
>> +       return 0;
> 
> ...we can't assume success when we cannot check the memory provider.
> The memory provider check is somewhat critical; we rely on it to
> detect that the driver does not support memory providers or is not
> doing the right thing, and report that to the user. I don't think we
> can silently disable the check when the CONFIG_SYSFS is disabled.
> Please return -ENODATA or some other error here.

I initially returned 0 to have the same behaviour as when
CONFIG_PAGE_POOL is not defined. But thanks to your explanations, I
understand it seems better to return -ENODATA here. Or another errno, to
let the userspace understanding there is a different error? I can send a
v2 after the 24h rate-limit period if you are OK with that.

> If we disable devmem TCP for !CONFIG_SYSFS we should probably add
> something to the docs saying this. I can do that in a follow up
> change.

Good point, thank you.

> However, I'm looking at the definition of get_netdev_rx_queue_index()
> and at first glance I don't see anything there that is actually
> dependent on CONFIG_SYSFS. Can we do this instead? I have build-tested
> it and it resolves the build issue as well:
> 
> ```
> diff --git a/include/net/netdev_rx_queue.h b/include/net/netdev_rx_queue.h
> index ac34f5fb4f71..596836abf7bf 100644
> --- a/include/net/netdev_rx_queue.h
> +++ b/include/net/netdev_rx_queue.h
> @@ -45,7 +45,6 @@ __netif_get_rx_queue(struct net_device *dev, unsigned int rxq)
>         return dev->_rx + rxq;
>  }
> 
> -#ifdef CONFIG_SYSFS
>  static inline unsigned int
>  get_netdev_rx_queue_index(struct netdev_rx_queue *queue)
>  {
> @@ -55,7 +54,6 @@ get_netdev_rx_queue_index(struct netdev_rx_queue *queue)
>         BUG_ON(index >= dev->num_rx_queues);
>         return index;
>  }
> -#endif
>  ```

I briefly looked at taking this path when I saw what this helper was
doing, but then I saw all operations related to the received queues were
enabled only when CONFIG_SYSFS is set, see commit a953be53ce40
("net-sysfs: add support for device-specific rx queue sysfs
attributes"). I understood from that it is better not to look at
dev->_rx or dev->num_rx_queues when CONFIG_SYSFS is not set. I'm not
very familiar to that part of the code, but it feels like removing this
#ifdef might be similar to the "return 0" I suggested: silently
disabling the check, no?

I *think* it might be clearer to return an error when SYSFS is not set.

> Matthieu, I'm happy to follow up with v2 of this fix if you don't have time.
If you prefer to explore other ways than returning an error in
page_pool_check_memory_provider() when SYSFS is not set, yes please do
the follow-up if you don't mind. My main goal is to stop my CI to
complain about that when compiling with 'make tinyconfig' + MPTCP :)

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Powered by blists - more mailing lists