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

[Jakub Kicinski <kuba@kernel.org>]

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?

> @@ -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.

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

> 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

>  #include <linux/uaccess.h>
>  #include <asm/ioctls.h>
> @@ -2476,6 +2477,11 @@ static int tcp_recvmsg_dmabuf(struct sock *sk, const struct sk_buff *skb,
>  			}
>  
>  			niov = skb_frag_net_iov(frag);
> +			if (net_is_devmem_page_pool_ops(niov->pp->mp_ops)) {

This one seems legit to me, FWIW, checking if its devmem in devmem
specific code is fine. 

> +				err = -ENODEV;
> +				goto out;
> +			}
> +
>  			end = start + skb_frag_size(frag);
>  			copy = end - offset;
>