Re: [PATCH net-next v3 3/8] virtio-net: xsk zero copy xmit setup

* Re: [PATCH net-next v3 3/8] virtio-net: xsk zero copy xmit setup
       [not found] <1617780476.5300975-1-xuanzhuo@linux.alibaba.com>
@ 2021-04-07  9:00 ` Jason Wang
  0 siblings, 0 replies; 2+ messages in thread
From: Jason Wang @ 2021-04-07  9:00 UTC (permalink / raw)
  To: Xuan Zhuo
  Cc: Song Liu, Martin KaFai Lau, Jesper Dangaard Brouer,
	Daniel Borkmann, Michael S. Tsirkin, Yonghong Song,
	John Fastabend, Alexei Starovoitov, Andrii Nakryiko, netdev,
	Björn Töpel, Dust Li, Jonathan Lemon, KP Singh,
	Jakub Kicinski, bpf, virtualization, David S. Miller,
	Magnus Karlsson

在 2021/4/7 下午3:27, Xuan Zhuo 写道:
> On Tue, 6 Apr 2021 12:27:14 +0800, Jason Wang <jasowang@redhat.com> wrote:
>> 在 2021/3/31 下午3:11, Xuan Zhuo 写道:
>>> xsk is a high-performance packet receiving and sending technology.
>>>
>>> This patch implements the binding and unbinding operations of xsk and
>>> the virtio-net queue for xsk zero copy xmit.
>>>
>>> The xsk zero copy xmit depends on tx napi. So if tx napi is not opened,
>>> an error will be reported. And the entire operation is under the
>>> protection of rtnl_lock
>>>
>>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>> Reviewed-by: Dust Li <dust.li@linux.alibaba.com>
>>> ---
>>>    drivers/net/virtio_net.c | 66 ++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 66 insertions(+)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index bb4ea9dbc16b..4e25408a2b37 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -22,6 +22,7 @@
>>>    #include <net/route.h>
>>>    #include <net/xdp.h>
>>>    #include <net/net_failover.h>
>>> +#include <net/xdp_sock_drv.h>
>>>
>>>    static int napi_weight = NAPI_POLL_WEIGHT;
>>>    module_param(napi_weight, int, 0444);
>>> @@ -133,6 +134,11 @@ struct send_queue {
>>>    	struct virtnet_sq_stats stats;
>>>
>>>    	struct napi_struct napi;
>>> +
>>> +	struct {
>>> +		/* xsk pool */
>>> +		struct xsk_buff_pool __rcu *pool;
>>> +	} xsk;
>>>    };
>>>
>>>    /* Internal representation of a receive virtqueue */
>>> @@ -2526,11 +2532,71 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>>>    	return err;
>>>    }
>>>
>>> +static int virtnet_xsk_pool_enable(struct net_device *dev,
>>> +				   struct xsk_buff_pool *pool,
>>> +				   u16 qid)
>>> +{
>>> +	struct virtnet_info *vi = netdev_priv(dev);
>>> +	struct send_queue *sq;
>>> +	int ret = -EBUSY;
>>
>> I'd rather move this under the check of xsk.pool.
>>
>>
>>> +
>>> +	if (qid >= vi->curr_queue_pairs)
>>> +		return -EINVAL;
>>> +
>>> +	sq = &vi->sq[qid];
>>> +
>>> +	/* xsk zerocopy depend on the tx napi */
>>
>> Need more comments to explain why tx NAPI is required here.
>>
>> And what's more important, tx NAPI could be enabled/disable via ethtool,
>> what if the NAPI is disabled after xsk is enabled?
>>
> If napi_tx is turned off, then the xmit will be affected.

Please document what kind of effect that prevents xsk from working.

> Maybe I
> should restrict that tx NAPI be disable via ethtool after xsk is enabled.

It can work.

>
>>> +	if (!sq->napi.weight)
>>> +		return -EPERM;
>>> +
>>> +	rcu_read_lock();
>>> +	if (rcu_dereference(sq->xsk.pool))
>>> +		goto end;
>>
>> Under what condition can we reach here?
> When the user tries to bind repeatedly.

Ok, I am a little suprised that it was not checked by xsk_bind().

>
>>
>>> +
>>> +	/* Here is already protected by rtnl_lock, so rcu_assign_pointer is
>>> +	 * safe.
>>> +	 */
>>> +	rcu_assign_pointer(sq->xsk.pool, pool);
>>> +	ret = 0;
>>> +end:
>>> +	rcu_read_unlock();
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int virtnet_xsk_pool_disable(struct net_device *dev, u16 qid)
>>> +{
>>> +	struct virtnet_info *vi = netdev_priv(dev);
>>> +	struct send_queue *sq;
>>> +
>>> +	if (qid >= vi->curr_queue_pairs)
>>> +		return -EINVAL;
>>> +
>>> +	sq = &vi->sq[qid];
>>> +
>>> +	/* Here is already protected by rtnl_lock, so rcu_assign_pointer is
>>> +	 * safe.
>>> +	 */
>>> +	rcu_assign_pointer(sq->xsk.pool, NULL);
>>> +
>>> +	synchronize_rcu(); /* Sync with the XSK wakeup and with NAPI. */
>>
>> Since rtnl is held here, I guess it's better to use synchornize_net().
>>
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>    static int virtnet_xdp(struct net_device *dev, struct netdev_bpf *xdp)
>>>    {
>>>    	switch (xdp->command) {
>>>    	case XDP_SETUP_PROG:
>>>    		return virtnet_xdp_set(dev, xdp->prog, xdp->extack);
>>> +	case XDP_SETUP_XSK_POOL:
>>> +		/* virtio net not use dma before call vring api */
>>> +		xdp->xsk.check_dma = false;
>>
>> I think it's better not open code things like this. How about introduce
>> new parameters in xp_assign_dev()?
> xp_assign_dev is called by the user, we should let xsk perceive that the current
> dev does not directly use dma. Is it possible to use dev->priv_flags? I only
> know that this is the case with virtio-net!!

Ok, then it should be fine, but we need a better name other than 
"check_dma". Maybe "use_dma_addr" instead.

Thanks

>
> Thanks very much.
>
>>
>>> +		if (xdp->xsk.pool)
>>> +			return virtnet_xsk_pool_enable(dev, xdp->xsk.pool,
>>> +						       xdp->xsk.queue_id);
>>> +		else
>>> +			return virtnet_xsk_pool_disable(dev, xdp->xsk.queue_id);
>>>    	default:
>>>    		return -EINVAL;
>>>    	}
>>
>> Thanks
>>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 2+ messages in thread