netdev - Re: [PATCH net-next v8 04/17] net: prepare for non devmem TCP memory providers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <12cb04de-dfbe-4247-b1d6-8e6feae640d8@gmail.com>
Date: Tue, 10 Dec 2024 03:53:36 +0000
From: Pavel Begunkov <asml.silence@...il.com>
To: Jakub Kicinski <kuba@...nel.org>, David Wei <dw@...idwei.uk>
Cc: io-uring@...r.kernel.org, netdev@...r.kernel.org,
 Jens Axboe <axboe@...nel.dk>, Paolo Abeni <pabeni@...hat.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jesper Dangaard Brouer <hawk@...nel.org>, David Ahern <dsahern@...nel.org>,
 Mina Almasry <almasrymina@...gle.com>,
 Stanislav Fomichev <stfomichev@...il.com>, Joe Damato <jdamato@...tly.com>,
 Pedro Tammela <pctammela@...atatu.com>
Subject: Re: [PATCH net-next v8 04/17] net: prepare for non devmem TCP memory
 providers

On 12/10/24 03:15, Jakub Kicinski wrote:
> On Wed,  4 Dec 2024 09:21:43 -0800 David Wei wrote:
>> +EXPORT_SYMBOL_GPL(net_is_devmem_page_pool_ops);
> 
> Export doesn't seem necessary, no module should need this right?

I remembered TCP can be modularised, but seems CONFIG_INET is
bool. Then yeah, can be killed.

>> @@ -316,10 +322,10 @@ void dev_dmabuf_uninstall(struct net_device *dev)
>>   	unsigned int i;
>>   
>>   	for (i = 0; i < dev->real_num_rx_queues; i++) {
>> -		binding = dev->_rx[i].mp_params.mp_priv;
>> -		if (!binding)
>> +		if (dev->_rx[i].mp_params.mp_ops != &dmabuf_devmem_ops)
>>   			continue;
>>   
>> +		binding = dev->_rx[i].mp_params.mp_priv;
>>   		xa_for_each(&binding->bound_rxqs, xa_idx, rxq)
>>   			if (rxq == &dev->_rx[i]) {
>>   				xa_erase(&binding->bound_rxqs, xa_idx);
> 
> Maybe add an op to mp_ops for queue unbinding?
> Having an op struct and yet running code under if (ops == X) seems odd.

ok

>> -	if (binding && nla_put_u32(rsp, NETDEV_A_PAGE_POOL_DMABUF, binding->id))
>> -		goto err_cancel;
>> +	if (net_is_devmem_page_pool_ops(pool->mp_ops)) {
>> +		binding = pool->mp_priv;
>> +		if (nla_put_u32(rsp, NETDEV_A_PAGE_POOL_DMABUF, binding->id))
>> +			goto err_cancel;
> 
> ditto, all mps should show up in page pool info. Even if it's just
> an empty nest for now, waiting for attributes to be added later.
> 
>> +	}
>>   
>>   	genlmsg_end(rsp, hdr);
>>   
>> @@ -353,16 +356,16 @@ void page_pool_unlist(struct page_pool *pool)
>>   int page_pool_check_memory_provider(struct net_device *dev,
>>   				    struct netdev_rx_queue *rxq)
>>   {
>> -	struct net_devmem_dmabuf_binding *binding = rxq->mp_params.mp_priv;
>> +	void *mp_priv = rxq->mp_params.mp_priv;
>>   	struct page_pool *pool;
>>   	struct hlist_node *n;
>>   
>> -	if (!binding)
>> +	if (!mp_priv)
>>   		return 0;
>>   
>>   	mutex_lock(&page_pools_lock);
>>   	hlist_for_each_entry_safe(pool, n, &dev->page_pools, user.list) {
>> -		if (pool->mp_priv != binding)
>> +		if (pool->mp_priv != mp_priv)
>>   			continue;
>>   
>>   		if (pool->slow.queue_idx == get_netdev_rx_queue_index(rxq)) {
> 
> appears to be unrelated

The entire chunk? It removes the type, nobody should be blindly casting
it to devmem specific binding even if it's not referenced, otherwise it
gets pretty ugly pretty fast. E.g. people might assume that it's always
the right type to cast to.

>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
>> index b872de9a8271..f22005c70fd3 100644
>> --- a/net/ipv4/tcp.c
>> +++ b/net/ipv4/tcp.c
>> @@ -277,6 +277,7 @@
>>   #include <net/ip.h>
>>   #include <net/sock.h>
>>   #include <net/rstreason.h>
>> +#include <net/page_pool/types.h>
> 
> types.h being needed to call a helper is unusual

it's in devmem.h, so types.h shouldn't be needed indeed

-- 
Pavel Begunkov