[PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[David Ahern]

From: David Ahern <dahern@digitalocean.com>

virtio_net currently requires extra queues to install an XDP program,
with the rule being twice as many queues as vcpus. From a host
perspective this means the VM needs to have 2*vcpus vhost threads
for each guest NIC for which XDP is to be allowed. For example, a
16 vcpu VM with 2 tap devices needs 64 vhost threads.

The extra queues are only needed in case an XDP program wants to
return XDP_TX. XDP_PASS, XDP_DROP and XDP_REDIRECT do not need
additional queues. Relax the queue requirement and allow XDP
functionality based on resources. If an XDP program is loaded and
there are insufficient queues, then return a warning to the user
and if a program returns XDP_TX just drop the packet. This allows
the use of the rest of the XDP functionality to work without
putting an unreasonable burden on the host.

Cc: Jason Wang <jasowang@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: David Ahern <dahern@digitalocean.com>
---
 drivers/net/virtio_net.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 2fe7a3188282..2f4c5b2e674d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -190,6 +190,8 @@ struct virtnet_info {
 	/* # of XDP queue pairs currently used by the driver */
 	u16 xdp_queue_pairs;
 
+	bool can_do_xdp_tx;
+
 	/* I like... big packets and I cannot lie! */
 	bool big_packets;
 
@@ -697,6 +699,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
 			len = xdp.data_end - xdp.data;
 			break;
 		case XDP_TX:
+			if (!vi->can_do_xdp_tx)
+				goto err_xdp;
 			stats->xdp_tx++;
 			xdpf = convert_to_xdp_frame(&xdp);
 			if (unlikely(!xdpf))
@@ -870,6 +874,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 			}
 			break;
 		case XDP_TX:
+			if (!vi->can_do_xdp_tx)
+				goto err_xdp;
 			stats->xdp_tx++;
 			xdpf = convert_to_xdp_frame(&xdp);
 			if (unlikely(!xdpf))
@@ -2435,10 +2441,10 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 
 	/* XDP requires extra queues for XDP_TX */
 	if (curr_qp + xdp_qp > vi->max_queue_pairs) {
-		NL_SET_ERR_MSG_MOD(extack, "Too few free TX rings available");
-		netdev_warn(dev, "request %i queues but max is %i\n",
-			    curr_qp + xdp_qp, vi->max_queue_pairs);
-		return -ENOMEM;
+		NL_SET_ERR_MSG_MOD(extack, "Too few free TX rings available; XDP_TX will not be allowed");
+		vi->can_do_xdp_tx = false;
+	} else {
+		vi->can_do_xdp_tx = true;
 	}
 
 	old_prog = rtnl_dereference(vi->rq[0].xdp_prog);
-- 
2.17.1

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[Jakub Kicinski]

On Tue, 25 Feb 2020 17:57:44 -0700 David Ahern wrote:
> From: David Ahern <dahern@digitalocean.com>
> 
> virtio_net currently requires extra queues to install an XDP program,
> with the rule being twice as many queues as vcpus. From a host
> perspective this means the VM needs to have 2*vcpus vhost threads
> for each guest NIC for which XDP is to be allowed. For example, a
> 16 vcpu VM with 2 tap devices needs 64 vhost threads.
> 
> The extra queues are only needed in case an XDP program wants to
> return XDP_TX. XDP_PASS, XDP_DROP and XDP_REDIRECT do not need
> additional queues. Relax the queue requirement and allow XDP
> functionality based on resources. If an XDP program is loaded and
> there are insufficient queues, then return a warning to the user
> and if a program returns XDP_TX just drop the packet. This allows
> the use of the rest of the XDP functionality to work without
> putting an unreasonable burden on the host.
> 
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: David Ahern <dahern@digitalocean.com>

The plan^W hope in my head was that Magnus will expose more info about
NIC queues. Then we can introduce a "manual queue setup" mode for XDP,
where users is responsible for making sure that the cores they care
about have TX/REDIRECT queues (with or without an XDP program attached).
Right now the XDP queues are entirely invisible.

That's just FWIW, in absence of actual code - regardless if it's an
obvious or pointless idea - it may not have much weight in this
discussion..

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[Jason Wang]

On 2020/2/26 上午8:57, David Ahern wrote:
> From: David Ahern <dahern@digitalocean.com>
>
> virtio_net currently requires extra queues to install an XDP program,
> with the rule being twice as many queues as vcpus. From a host
> perspective this means the VM needs to have 2*vcpus vhost threads
> for each guest NIC for which XDP is to be allowed. For example, a
> 16 vcpu VM with 2 tap devices needs 64 vhost threads.
>
> The extra queues are only needed in case an XDP program wants to
> return XDP_TX. XDP_PASS, XDP_DROP and XDP_REDIRECT do not need
> additional queues. Relax the queue requirement and allow XDP
> functionality based on resources. If an XDP program is loaded and
> there are insufficient queues, then return a warning to the user
> and if a program returns XDP_TX just drop the packet. This allows
> the use of the rest of the XDP functionality to work without
> putting an unreasonable burden on the host.
>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: David Ahern <dahern@digitalocean.com>
> ---
>   drivers/net/virtio_net.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 2fe7a3188282..2f4c5b2e674d 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -190,6 +190,8 @@ struct virtnet_info {
>   	/* # of XDP queue pairs currently used by the driver */
>   	u16 xdp_queue_pairs;
>   
> +	bool can_do_xdp_tx;
> +
>   	/* I like... big packets and I cannot lie! */
>   	bool big_packets;
>   
> @@ -697,6 +699,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
>   			len = xdp.data_end - xdp.data;
>   			break;
>   		case XDP_TX:
> +			if (!vi->can_do_xdp_tx)
> +				goto err_xdp;


I wonder if using spinlock to synchronize XDP_TX is better than dropping 
here?

Thanks


>   			stats->xdp_tx++;
>   			xdpf = convert_to_xdp_frame(&xdp);
>   			if (unlikely(!xdpf))
> @@ -870,6 +874,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>   			}
>   			break;
>   		case XDP_TX:
> +			if (!vi->can_do_xdp_tx)
> +				goto err_xdp;
>   			stats->xdp_tx++;
>   			xdpf = convert_to_xdp_frame(&xdp);
>   			if (unlikely(!xdpf))
> @@ -2435,10 +2441,10 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>   
>   	/* XDP requires extra queues for XDP_TX */
>   	if (curr_qp + xdp_qp > vi->max_queue_pairs) {
> -		NL_SET_ERR_MSG_MOD(extack, "Too few free TX rings available");
> -		netdev_warn(dev, "request %i queues but max is %i\n",
> -			    curr_qp + xdp_qp, vi->max_queue_pairs);
> -		return -ENOMEM;
> +		NL_SET_ERR_MSG_MOD(extack, "Too few free TX rings available; XDP_TX will not be allowed");
> +		vi->can_do_xdp_tx = false;
> +	} else {
> +		vi->can_do_xdp_tx = true;
>   	}
>   
>   	old_prog = rtnl_dereference(vi->rq[0].xdp_prog);

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[David Ahern]

On 2/25/20 8:00 PM, Jason Wang wrote:
> 
> On 2020/2/26 上午8:57, David Ahern wrote:
>> From: David Ahern <dahern@digitalocean.com>
>>
>> virtio_net currently requires extra queues to install an XDP program,
>> with the rule being twice as many queues as vcpus. From a host
>> perspective this means the VM needs to have 2*vcpus vhost threads
>> for each guest NIC for which XDP is to be allowed. For example, a
>> 16 vcpu VM with 2 tap devices needs 64 vhost threads.
>>
>> The extra queues are only needed in case an XDP program wants to
>> return XDP_TX. XDP_PASS, XDP_DROP and XDP_REDIRECT do not need
>> additional queues. Relax the queue requirement and allow XDP
>> functionality based on resources. If an XDP program is loaded and
>> there are insufficient queues, then return a warning to the user
>> and if a program returns XDP_TX just drop the packet. This allows
>> the use of the rest of the XDP functionality to work without
>> putting an unreasonable burden on the host.
>>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: David Ahern <dahern@digitalocean.com>
>> ---
>>   drivers/net/virtio_net.c | 14 ++++++++++----
>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 2fe7a3188282..2f4c5b2e674d 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -190,6 +190,8 @@ struct virtnet_info {
>>       /* # of XDP queue pairs currently used by the driver */
>>       u16 xdp_queue_pairs;
>>   +    bool can_do_xdp_tx;
>> +
>>       /* I like... big packets and I cannot lie! */
>>       bool big_packets;
>>   @@ -697,6 +699,8 @@ static struct sk_buff *receive_small(struct
>> net_device *dev,
>>               len = xdp.data_end - xdp.data;
>>               break;
>>           case XDP_TX:
>> +            if (!vi->can_do_xdp_tx)
>> +                goto err_xdp;
> 
> 
> I wonder if using spinlock to synchronize XDP_TX is better than dropping
> here?

I recall you suggesting that. Sure, it makes for a friendlier user
experience, but if a spinlock makes this slower then it goes against the
core idea of XDP.


> 
> Thanks
> 
> 
>>               stats->xdp_tx++;
>>               xdpf = convert_to_xdp_frame(&xdp);
>>               if (unlikely(!xdpf))
>> @@ -870,6 +874,8 @@ static struct sk_buff *receive_mergeable(struct
>> net_device *dev,
>>               }
>>               break;
>>           case XDP_TX:
>> +            if (!vi->can_do_xdp_tx)
>> +                goto err_xdp;
>>               stats->xdp_tx++;
>>               xdpf = convert_to_xdp_frame(&xdp);
>>               if (unlikely(!xdpf))
>> @@ -2435,10 +2441,10 @@ static int virtnet_xdp_set(struct net_device
>> *dev, struct bpf_prog *prog,
>>         /* XDP requires extra queues for XDP_TX */
>>       if (curr_qp + xdp_qp > vi->max_queue_pairs) {
>> -        NL_SET_ERR_MSG_MOD(extack, "Too few free TX rings available");
>> -        netdev_warn(dev, "request %i queues but max is %i\n",
>> -                curr_qp + xdp_qp, vi->max_queue_pairs);
>> -        return -ENOMEM;
>> +        NL_SET_ERR_MSG_MOD(extack, "Too few free TX rings available;
>> XDP_TX will not be allowed");
>> +        vi->can_do_xdp_tx = false;
>> +    } else {
>> +        vi->can_do_xdp_tx = true;
>>       }
>>         old_prog = rtnl_dereference(vi->rq[0].xdp_prog);
> 

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[Jason Wang]

On 2020/2/26 上午11:24, David Ahern wrote:
> On 2/25/20 8:00 PM, Jason Wang wrote:
>> On 2020/2/26 上午8:57, David Ahern wrote:
>>> From: David Ahern<dahern@digitalocean.com>
>>>
>>> virtio_net currently requires extra queues to install an XDP program,
>>> with the rule being twice as many queues as vcpus. From a host
>>> perspective this means the VM needs to have 2*vcpus vhost threads
>>> for each guest NIC for which XDP is to be allowed. For example, a
>>> 16 vcpu VM with 2 tap devices needs 64 vhost threads.
>>>
>>> The extra queues are only needed in case an XDP program wants to
>>> return XDP_TX. XDP_PASS, XDP_DROP and XDP_REDIRECT do not need
>>> additional queues. Relax the queue requirement and allow XDP
>>> functionality based on resources. If an XDP program is loaded and
>>> there are insufficient queues, then return a warning to the user
>>> and if a program returns XDP_TX just drop the packet. This allows
>>> the use of the rest of the XDP functionality to work without
>>> putting an unreasonable burden on the host.
>>>
>>> Cc: Jason Wang<jasowang@redhat.com>
>>> Cc: Michael S. Tsirkin<mst@redhat.com>
>>> Signed-off-by: David Ahern<dahern@digitalocean.com>
>>> ---
>>>    drivers/net/virtio_net.c | 14 ++++++++++----
>>>    1 file changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index 2fe7a3188282..2f4c5b2e674d 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -190,6 +190,8 @@ struct virtnet_info {
>>>        /* # of XDP queue pairs currently used by the driver */
>>>        u16 xdp_queue_pairs;
>>>    +    bool can_do_xdp_tx;
>>> +
>>>        /* I like... big packets and I cannot lie! */
>>>        bool big_packets;
>>>    @@ -697,6 +699,8 @@ static struct sk_buff *receive_small(struct
>>> net_device *dev,
>>>                len = xdp.data_end - xdp.data;
>>>                break;
>>>            case XDP_TX:
>>> +            if (!vi->can_do_xdp_tx)
>>> +                goto err_xdp;
>> I wonder if using spinlock to synchronize XDP_TX is better than dropping
>> here?
> I recall you suggesting that. Sure, it makes for a friendlier user
> experience, but if a spinlock makes this slower then it goes against the
> core idea of XDP.
>
>

Maybe we can do some benchmark. TAP uses spinlock for XDP_TX. If my 
memory is correct, for the best case (no queue contention), it can only 
have ~10% PPS drop on heavy workload.

Thanks

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[David Ahern]

On 2/25/20 8:29 PM, Jason Wang wrote:
> TAP uses spinlock for XDP_TX.

code reference? I can not find that.

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[Jason Wang]

On 2020/2/26 上午11:34, David Ahern wrote:
> On 2/25/20 8:29 PM, Jason Wang wrote:
>> TAP uses spinlock for XDP_TX.
> code reference? I can not find that.
>

In tun_xdp_xmit(), ptr_ring is synchronized through producer_lock.

Thanks

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[David Ahern]

On 2/25/20 8:52 PM, Jason Wang wrote:
> 
> On 2020/2/26 上午11:34, David Ahern wrote:
>> On 2/25/20 8:29 PM, Jason Wang wrote:
>>> TAP uses spinlock for XDP_TX.
>> code reference? I can not find that.
>>
> 
> In tun_xdp_xmit(), ptr_ring is synchronized through producer_lock.
> 

thanks. I was confused by the tap comment when it is the tun code.

So you mean a spinlock around virtnet_xdp_xmit for XDP_TX:

+                       if (!vi->can_do_xdp_tx)
+                               spin_lock(&vi->xdp_tx_lock);
                        err = virtnet_xdp_xmit(dev, 1, &xdpf, 0);
+                       if (!vi->can_do_xdp_tx)
+                               spin_unlock(&vi->xdp_tx_lock);

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[Jason Wang]

On 2020/2/26 下午12:35, David Ahern wrote:
> On 2/25/20 8:52 PM, Jason Wang wrote:
>> On 2020/2/26 上午11:34, David Ahern wrote:
>>> On 2/25/20 8:29 PM, Jason Wang wrote:
>>>> TAP uses spinlock for XDP_TX.
>>> code reference? I can not find that.
>>>
>> In tun_xdp_xmit(), ptr_ring is synchronized through producer_lock.
>>
> thanks. I was confused by the tap comment when it is the tun code.


Ah, I see, yes, the tap is kind of confusing.


>
> So you mean a spinlock around virtnet_xdp_xmit for XDP_TX:
>
> +                       if (!vi->can_do_xdp_tx)
> +                               spin_lock(&vi->xdp_tx_lock);
>                          err = virtnet_xdp_xmit(dev, 1, &xdpf, 0);
> +                       if (!vi->can_do_xdp_tx)
> +                               spin_unlock(&vi->xdp_tx_lock);


Something like this, but probably need to use __netif_tx_lock_bh() 
inside virtnet_xdp_xmit().

Need to calculate a sendqueue before in virtnet_xdp_sq().

Thanks

>

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[Toke Høiland-Jørgensen]

David Ahern <dsahern@gmail.com> writes:

> On 2/25/20 8:00 PM, Jason Wang wrote:
>> 
>> On 2020/2/26 上午8:57, David Ahern wrote:
>>> From: David Ahern <dahern@digitalocean.com>
>>>
>>> virtio_net currently requires extra queues to install an XDP program,
>>> with the rule being twice as many queues as vcpus. From a host
>>> perspective this means the VM needs to have 2*vcpus vhost threads
>>> for each guest NIC for which XDP is to be allowed. For example, a
>>> 16 vcpu VM with 2 tap devices needs 64 vhost threads.
>>>
>>> The extra queues are only needed in case an XDP program wants to
>>> return XDP_TX. XDP_PASS, XDP_DROP and XDP_REDIRECT do not need
>>> additional queues. Relax the queue requirement and allow XDP
>>> functionality based on resources. If an XDP program is loaded and
>>> there are insufficient queues, then return a warning to the user
>>> and if a program returns XDP_TX just drop the packet. This allows
>>> the use of the rest of the XDP functionality to work without
>>> putting an unreasonable burden on the host.
>>>
>>> Cc: Jason Wang <jasowang@redhat.com>
>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: David Ahern <dahern@digitalocean.com>
>>> ---
>>>   drivers/net/virtio_net.c | 14 ++++++++++----
>>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index 2fe7a3188282..2f4c5b2e674d 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -190,6 +190,8 @@ struct virtnet_info {
>>>       /* # of XDP queue pairs currently used by the driver */
>>>       u16 xdp_queue_pairs;
>>>   +    bool can_do_xdp_tx;
>>> +
>>>       /* I like... big packets and I cannot lie! */
>>>       bool big_packets;
>>>   @@ -697,6 +699,8 @@ static struct sk_buff *receive_small(struct
>>> net_device *dev,
>>>               len = xdp.data_end - xdp.data;
>>>               break;
>>>           case XDP_TX:
>>> +            if (!vi->can_do_xdp_tx)
>>> +                goto err_xdp;
>> 
>> 
>> I wonder if using spinlock to synchronize XDP_TX is better than dropping
>> here?
>
> I recall you suggesting that. Sure, it makes for a friendlier user
> experience, but if a spinlock makes this slower then it goes against the
> core idea of XDP.

IMO a spinlock-arbitrated TX queue is something that should be available
to the user if configured (using that queue abstraction Magnus is
working on), but not the default, since as you say that goes against the
"performance first" mantra of XDP.

-Toke

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[Michael S. Tsirkin]

On Wed, Feb 26, 2020 at 09:19:45AM +0100, Toke Høiland-Jørgensen wrote:
> David Ahern <dsahern@gmail.com> writes:
> 
> > On 2/25/20 8:00 PM, Jason Wang wrote:
> >> 
> >> On 2020/2/26 上午8:57, David Ahern wrote:
> >>> From: David Ahern <dahern@digitalocean.com>
> >>>
> >>> virtio_net currently requires extra queues to install an XDP program,
> >>> with the rule being twice as many queues as vcpus. From a host
> >>> perspective this means the VM needs to have 2*vcpus vhost threads
> >>> for each guest NIC for which XDP is to be allowed. For example, a
> >>> 16 vcpu VM with 2 tap devices needs 64 vhost threads.
> >>>
> >>> The extra queues are only needed in case an XDP program wants to
> >>> return XDP_TX. XDP_PASS, XDP_DROP and XDP_REDIRECT do not need
> >>> additional queues. Relax the queue requirement and allow XDP
> >>> functionality based on resources. If an XDP program is loaded and
> >>> there are insufficient queues, then return a warning to the user
> >>> and if a program returns XDP_TX just drop the packet. This allows
> >>> the use of the rest of the XDP functionality to work without
> >>> putting an unreasonable burden on the host.
> >>>
> >>> Cc: Jason Wang <jasowang@redhat.com>
> >>> Cc: Michael S. Tsirkin <mst@redhat.com>
> >>> Signed-off-by: David Ahern <dahern@digitalocean.com>
> >>> ---
> >>>   drivers/net/virtio_net.c | 14 ++++++++++----
> >>>   1 file changed, 10 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >>> index 2fe7a3188282..2f4c5b2e674d 100644
> >>> --- a/drivers/net/virtio_net.c
> >>> +++ b/drivers/net/virtio_net.c
> >>> @@ -190,6 +190,8 @@ struct virtnet_info {
> >>>       /* # of XDP queue pairs currently used by the driver */
> >>>       u16 xdp_queue_pairs;
> >>>   +    bool can_do_xdp_tx;
> >>> +
> >>>       /* I like... big packets and I cannot lie! */
> >>>       bool big_packets;
> >>>   @@ -697,6 +699,8 @@ static struct sk_buff *receive_small(struct
> >>> net_device *dev,
> >>>               len = xdp.data_end - xdp.data;
> >>>               break;
> >>>           case XDP_TX:
> >>> +            if (!vi->can_do_xdp_tx)
> >>> +                goto err_xdp;
> >> 
> >> 
> >> I wonder if using spinlock to synchronize XDP_TX is better than dropping
> >> here?
> >
> > I recall you suggesting that. Sure, it makes for a friendlier user
> > experience, but if a spinlock makes this slower then it goes against the
> > core idea of XDP.
> 
> IMO a spinlock-arbitrated TX queue is something that should be available
> to the user if configured (using that queue abstraction Magnus is
> working on), but not the default, since as you say that goes against the
> "performance first" mantra of XDP.
> 
> -Toke

OK so basically there would be commands to configure which TX queue is
used by XDP. With enough resources default is to use dedicated queues.
With not enough resources default is to fail binding xdp program
unless queues are specified. Does this sound reasonable?
It remains to define how are changes in TX queue config handled.
Should they just be disallowed as long as there's an active XDP program?

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[Toke Høiland-Jørgensen]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, Feb 26, 2020 at 09:19:45AM +0100, Toke Høiland-Jørgensen wrote:
>> David Ahern <dsahern@gmail.com> writes:
>> 
>> > On 2/25/20 8:00 PM, Jason Wang wrote:
>> >> 
>> >> On 2020/2/26 上午8:57, David Ahern wrote:
>> >>> From: David Ahern <dahern@digitalocean.com>
>> >>>
>> >>> virtio_net currently requires extra queues to install an XDP program,
>> >>> with the rule being twice as many queues as vcpus. From a host
>> >>> perspective this means the VM needs to have 2*vcpus vhost threads
>> >>> for each guest NIC for which XDP is to be allowed. For example, a
>> >>> 16 vcpu VM with 2 tap devices needs 64 vhost threads.
>> >>>
>> >>> The extra queues are only needed in case an XDP program wants to
>> >>> return XDP_TX. XDP_PASS, XDP_DROP and XDP_REDIRECT do not need
>> >>> additional queues. Relax the queue requirement and allow XDP
>> >>> functionality based on resources. If an XDP program is loaded and
>> >>> there are insufficient queues, then return a warning to the user
>> >>> and if a program returns XDP_TX just drop the packet. This allows
>> >>> the use of the rest of the XDP functionality to work without
>> >>> putting an unreasonable burden on the host.
>> >>>
>> >>> Cc: Jason Wang <jasowang@redhat.com>
>> >>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> >>> Signed-off-by: David Ahern <dahern@digitalocean.com>
>> >>> ---
>> >>>   drivers/net/virtio_net.c | 14 ++++++++++----
>> >>>   1 file changed, 10 insertions(+), 4 deletions(-)
>> >>>
>> >>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> >>> index 2fe7a3188282..2f4c5b2e674d 100644
>> >>> --- a/drivers/net/virtio_net.c
>> >>> +++ b/drivers/net/virtio_net.c
>> >>> @@ -190,6 +190,8 @@ struct virtnet_info {
>> >>>       /* # of XDP queue pairs currently used by the driver */
>> >>>       u16 xdp_queue_pairs;
>> >>>   +    bool can_do_xdp_tx;
>> >>> +
>> >>>       /* I like... big packets and I cannot lie! */
>> >>>       bool big_packets;
>> >>>   @@ -697,6 +699,8 @@ static struct sk_buff *receive_small(struct
>> >>> net_device *dev,
>> >>>               len = xdp.data_end - xdp.data;
>> >>>               break;
>> >>>           case XDP_TX:
>> >>> +            if (!vi->can_do_xdp_tx)
>> >>> +                goto err_xdp;
>> >> 
>> >> 
>> >> I wonder if using spinlock to synchronize XDP_TX is better than dropping
>> >> here?
>> >
>> > I recall you suggesting that. Sure, it makes for a friendlier user
>> > experience, but if a spinlock makes this slower then it goes against the
>> > core idea of XDP.
>> 
>> IMO a spinlock-arbitrated TX queue is something that should be available
>> to the user if configured (using that queue abstraction Magnus is
>> working on), but not the default, since as you say that goes against the
>> "performance first" mantra of XDP.
>> 
>> -Toke
>
> OK so basically there would be commands to configure which TX queue is
> used by XDP. With enough resources default is to use dedicated queues.
> With not enough resources default is to fail binding xdp program
> unless queues are specified. Does this sound reasonable?

Yeah, that was the idea. See this talk from LPC last year for more
details: https://linuxplumbersconf.org/event/4/contributions/462/

> It remains to define how are changes in TX queue config handled.
> Should they just be disallowed as long as there's an active XDP program?

Well that would depend on what is possible for each device, I guess? But
we already do block some reconfiguration if an XDP program is loaded
(such as MTU changes), so there is some precedence for that :)

-Toke

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[Michael S. Tsirkin]

On Wed, Feb 26, 2020 at 09:34:51AM +0100, Toke Høiland-Jørgensen wrote:
> we already do block some reconfiguration if an XDP program is loaded
> (such as MTU changes), so there is some precedence for that :)
Do we really block MTU changes when XDP is loaded?
Seems to work for me - there's a separate discussion going
on about how to handle MTU changes - see
https://lore.kernel.org/r/7df5bb7f-ea69-7673-642b-f174e45a1e64@digitalocean.com
	virtio_net: can change MTU after installing program

-- 
MST

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[Toke Høiland-Jørgensen]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, Feb 26, 2020 at 09:34:51AM +0100, Toke Høiland-Jørgensen wrote:
>> we already do block some reconfiguration if an XDP program is loaded
>> (such as MTU changes), so there is some precedence for that :)
> Do we really block MTU changes when XDP is loaded?
> Seems to work for me - there's a separate discussion going
> on about how to handle MTU changes - see
> https://lore.kernel.org/r/7df5bb7f-ea69-7673-642b-f174e45a1e64@digitalocean.com
> 	virtio_net: can change MTU after installing program

Maybe not for virtio_net, but that same thing doesn't work on mlx5:

$ sudo ip link set dev ens1f1 mtu 8192
RTNETLINK answers: Invalid argument

-Toke

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[Michael S. Tsirkin]

On Wed, Feb 26, 2020 at 10:02:03AM +0100, Toke Høiland-Jørgensen wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Wed, Feb 26, 2020 at 09:34:51AM +0100, Toke Høiland-Jørgensen wrote:
> >> we already do block some reconfiguration if an XDP program is loaded
> >> (such as MTU changes), so there is some precedence for that :)
> > Do we really block MTU changes when XDP is loaded?
> > Seems to work for me - there's a separate discussion going
> > on about how to handle MTU changes - see
> > https://lore.kernel.org/r/7df5bb7f-ea69-7673-642b-f174e45a1e64@digitalocean.com
> > 	virtio_net: can change MTU after installing program
> 
> Maybe not for virtio_net, but that same thing doesn't work on mlx5:
> 
> $ sudo ip link set dev ens1f1 mtu 8192
> RTNETLINK answers: Invalid argument
> 
> -Toke

Let's take it to that thread please. I'll CC you.

-- 
MST

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[David Ahern]

On 2/26/20 1:34 AM, Toke Høiland-Jørgensen wrote:
>>
>> OK so basically there would be commands to configure which TX queue is
>> used by XDP. With enough resources default is to use dedicated queues.
>> With not enough resources default is to fail binding xdp program
>> unless queues are specified. Does this sound reasonable?
> 
> Yeah, that was the idea. See this talk from LPC last year for more
> details: https://linuxplumbersconf.org/event/4/contributions/462/

 Hopefully such a design is only required for a program doing a Tx path
(XDP_TX or XDP_REDIRECT). i.e., a program just doing basic ACL, NAT, or
even encap, decap, should not have to do anything with Tx queues to load
and run the program.

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[Toke Høiland-Jørgensen]

David Ahern <dahern@digitalocean.com> writes:

> On 2/26/20 1:34 AM, Toke Høiland-Jørgensen wrote:
>>>
>>> OK so basically there would be commands to configure which TX queue is
>>> used by XDP. With enough resources default is to use dedicated queues.
>>> With not enough resources default is to fail binding xdp program
>>> unless queues are specified. Does this sound reasonable?
>> 
>> Yeah, that was the idea. See this talk from LPC last year for more
>> details: https://linuxplumbersconf.org/event/4/contributions/462/
>
>  Hopefully such a design is only required for a program doing a Tx path
> (XDP_TX or XDP_REDIRECT). i.e., a program just doing basic ACL, NAT, or
> even encap, decap, should not have to do anything with Tx queues to load
> and run the program.

No but they may want to configure RX queues (for CPU affinity, etc).
Ideally we'd want both (TX and RX) to be configurable through the same
interface.

-Toke

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[Michael S. Tsirkin]

On Wed, Feb 26, 2020 at 08:58:47AM -0700, David Ahern wrote:
> On 2/26/20 1:34 AM, Toke Høiland-Jørgensen wrote:
> >>
> >> OK so basically there would be commands to configure which TX queue is
> >> used by XDP. With enough resources default is to use dedicated queues.
> >> With not enough resources default is to fail binding xdp program
> >> unless queues are specified. Does this sound reasonable?
> > 
> > Yeah, that was the idea. See this talk from LPC last year for more
> > details: https://linuxplumbersconf.org/event/4/contributions/462/
> 
>  Hopefully such a design is only required for a program doing a Tx path
> (XDP_TX or XDP_REDIRECT). i.e., a program just doing basic ACL, NAT, or
> even encap, decap, should not have to do anything with Tx queues to load
> and run the program.

Well when XDP was starting up it wasn't too late to require
meta data about which codes can be returned (e.g. whether program
can do tx). But by now there's a body of binary programs out there,
it's probably too late ...


-- 
MST

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[Toke Høiland-Jørgensen]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, Feb 26, 2020 at 08:58:47AM -0700, David Ahern wrote:
>> On 2/26/20 1:34 AM, Toke Høiland-Jørgensen wrote:
>> >>
>> >> OK so basically there would be commands to configure which TX queue is
>> >> used by XDP. With enough resources default is to use dedicated queues.
>> >> With not enough resources default is to fail binding xdp program
>> >> unless queues are specified. Does this sound reasonable?
>> > 
>> > Yeah, that was the idea. See this talk from LPC last year for more
>> > details: https://linuxplumbersconf.org/event/4/contributions/462/
>> 
>>  Hopefully such a design is only required for a program doing a Tx path
>> (XDP_TX or XDP_REDIRECT). i.e., a program just doing basic ACL, NAT, or
>> even encap, decap, should not have to do anything with Tx queues to load
>> and run the program.
>
> Well when XDP was starting up it wasn't too late to require
> meta data about which codes can be returned (e.g. whether program
> can do tx). But by now there's a body of binary programs out there,
> it's probably too late ...

Well, right now things just fail silently if the system is configured
without support for a feature the XDP program is using (e.g., redirect
to an unsupported iface will just drop the packet). So arguably,
rejecting a program is an improvement :) There's ongoing work to define
a notion of XDP features (see [0]). Whether we can turn it on by
default, or if it has to be opt-in on program load/attach remains to be
seem. But it is definitely something we should improve upon :)

-Toke

[0] See https://github.com/xdp-project/xdp-project/blob/master/xdp-project.org#xdp-feature-flags

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[David Ahern]

On 2/26/20 1:34 AM, Toke Høiland-Jørgensen wrote:
>> OK so basically there would be commands to configure which TX queue is
>> used by XDP. With enough resources default is to use dedicated queues.
>> With not enough resources default is to fail binding xdp program
>> unless queues are specified. Does this sound reasonable?
> Yeah, that was the idea. See this talk from LPC last year for more
> details: https://linuxplumbersconf.org/event/4/contributions/462/
> 

Can we use one of the breakout timeslots at netdevconf and discuss this
proposal and status?

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[Toke Høiland-Jørgensen]

David Ahern <dahern@digitalocean.com> writes:

> On 2/26/20 1:34 AM, Toke Høiland-Jørgensen wrote:
>>> OK so basically there would be commands to configure which TX queue is
>>> used by XDP. With enough resources default is to use dedicated queues.
>>> With not enough resources default is to fail binding xdp program
>>> unless queues are specified. Does this sound reasonable?
>> Yeah, that was the idea. See this talk from LPC last year for more
>> details: https://linuxplumbersconf.org/event/4/contributions/462/
>> 
>
> Can we use one of the breakout timeslots at netdevconf and discuss this
> proposal and status?

Adding in Magnus and Jesper; I won't be at netdevconf, sadly, but you
guys go ahead :)

Magnus indicated he may have some preliminary code to share soonish.
Maybe that means before netdevconf? ;)

-Toke

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[Magnus Karlsson]

On Thu, Feb 27, 2020 at 11:22 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> David Ahern <dahern@digitalocean.com> writes:
>
> > On 2/26/20 1:34 AM, Toke Høiland-Jørgensen wrote:
> >>> OK so basically there would be commands to configure which TX queue is
> >>> used by XDP. With enough resources default is to use dedicated queues.
> >>> With not enough resources default is to fail binding xdp program
> >>> unless queues are specified. Does this sound reasonable?
> >> Yeah, that was the idea. See this talk from LPC last year for more
> >> details: https://linuxplumbersconf.org/event/4/contributions/462/
> >>
> >
> > Can we use one of the breakout timeslots at netdevconf and discuss this
> > proposal and status?
>
> Adding in Magnus and Jesper; I won't be at netdevconf, sadly, but you
> guys go ahead :)
>
> Magnus indicated he may have some preliminary code to share soonish.
> Maybe that means before netdevconf? ;)

I will unfortunately be after Netdevconf due to other commitments. The
plan is to send out the RFC to the co-authors of the Plumbers
presentation first, just to check the sanity of it. And after that
send it to the mailing list. Note that I have taken two shortcuts in
the RFC to be able to make quicker progress. The first on is the
driver implementation of the dynamic queue allocation and
de-allocation. It just does this within a statically pre-allocated set
of queues. The second is that the user space interface is just a
setsockopt instead of a rtnetlink interface. Again, just to save some
time in this initial phase. The information communicated in the
interface is the same though. In the current code, the queue manager
can handle the queues of the networking stack, the XDP_TX queues and
queues allocated by user space and used for AF_XDP. Other uses from
user space is not covered due to my setsockopt shortcut. Hopefully
though, this should be enough for an initial assessment.

/Magnus

> -Toke
>r

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[David Ahern]

On 2/27/20 2:41 AM, Magnus Karlsson wrote:
> I will unfortunately be after Netdevconf due to other commitments. The
> plan is to send out the RFC to the co-authors of the Plumbers
> presentation first, just to check the sanity of it. And after that
> send it to the mailing list. Note that I have taken two shortcuts in
> the RFC to be able to make quicker progress. The first on is the
> driver implementation of the dynamic queue allocation and
> de-allocation. It just does this within a statically pre-allocated set
> of queues. The second is that the user space interface is just a
> setsockopt instead of a rtnetlink interface. Again, just to save some
> time in this initial phase. The information communicated in the
> interface is the same though. In the current code, the queue manager
> can handle the queues of the networking stack, the XDP_TX queues and
> queues allocated by user space and used for AF_XDP. Other uses from
> user space is not covered due to my setsockopt shortcut. Hopefully
> though, this should be enough for an initial assessment.

Any updates on the RFC? I do not recall seeing a patch set on the
mailing list, but maybe I missed it.

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[Magnus Karlsson]

On Mon, Sep 28, 2020 at 5:13 AM David Ahern <dsahern@gmail.com> wrote:
>
> On 2/27/20 2:41 AM, Magnus Karlsson wrote:
> > I will unfortunately be after Netdevconf due to other commitments. The
> > plan is to send out the RFC to the co-authors of the Plumbers
> > presentation first, just to check the sanity of it. And after that
> > send it to the mailing list. Note that I have taken two shortcuts in
> > the RFC to be able to make quicker progress. The first on is the
> > driver implementation of the dynamic queue allocation and
> > de-allocation. It just does this within a statically pre-allocated set
> > of queues. The second is that the user space interface is just a
> > setsockopt instead of a rtnetlink interface. Again, just to save some
> > time in this initial phase. The information communicated in the
> > interface is the same though. In the current code, the queue manager
> > can handle the queues of the networking stack, the XDP_TX queues and
> > queues allocated by user space and used for AF_XDP. Other uses from
> > user space is not covered due to my setsockopt shortcut. Hopefully
> > though, this should be enough for an initial assessment.
>
> Any updates on the RFC? I do not recall seeing a patch set on the
> mailing list, but maybe I missed it.

No, you have unfortunately not missed anything. It has been lying on
the shelf collecting dust for most of this time. The reason was that
the driver changes needed to support dynamic queue allocation just
became too complex as it would require major surgery to at least all
Intel drivers, and probably a large number of other ones as well. Do
not think any vendor would support such a high effort solution and I
could not (at that time at least) find a way around it. So, gaining
visibility into what queues have been allocated (by all entities in
the kernel that uses queue) seems to be rather straightforward, but
the dynamic allocation part seems to be anything but.

I also wonder how useful this queue manager proposal would be in light
of Mellanox's subfunction proposal. If people just start to create
many small netdevs (albeit at high cost which people may argue
against) consisting of just an rx/tx queue pair, then the queue
manager dynamic allocation proposal would not be as useful. We could
just use one of these netdevs to bind to in the AF_XDP case and always
just specify queue 0. But one can argue that queue management is
needed even for the subfunction approach, but then it would be at a
much lower level than what I proposed. What is your take on this?
Still worth pursuing in some form or another? If yes, then we really
need to come up with an easy way of supporting this in current
drivers. It is not going to fly otherwise, IMHO.

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[David Ahern]

On 9/28/20 7:25 AM, Magnus Karlsson wrote:
> On Mon, Sep 28, 2020 at 5:13 AM David Ahern <dsahern@gmail.com> wrote:
>>
>> On 2/27/20 2:41 AM, Magnus Karlsson wrote:
>>> I will unfortunately be after Netdevconf due to other commitments. The
>>> plan is to send out the RFC to the co-authors of the Plumbers
>>> presentation first, just to check the sanity of it. And after that
>>> send it to the mailing list. Note that I have taken two shortcuts in
>>> the RFC to be able to make quicker progress. The first on is the
>>> driver implementation of the dynamic queue allocation and
>>> de-allocation. It just does this within a statically pre-allocated set
>>> of queues. The second is that the user space interface is just a
>>> setsockopt instead of a rtnetlink interface. Again, just to save some
>>> time in this initial phase. The information communicated in the
>>> interface is the same though. In the current code, the queue manager
>>> can handle the queues of the networking stack, the XDP_TX queues and
>>> queues allocated by user space and used for AF_XDP. Other uses from
>>> user space is not covered due to my setsockopt shortcut. Hopefully
>>> though, this should be enough for an initial assessment.
>>
>> Any updates on the RFC? I do not recall seeing a patch set on the
>> mailing list, but maybe I missed it.
> 
> No, you have unfortunately not missed anything. It has been lying on
> the shelf collecting dust for most of this time. The reason was that
> the driver changes needed to support dynamic queue allocation just
> became too complex as it would require major surgery to at least all
> Intel drivers, and probably a large number of other ones as well. Do
> not think any vendor would support such a high effort solution and I
> could not (at that time at least) find a way around it. So, gaining
> visibility into what queues have been allocated (by all entities in
> the kernel that uses queue) seems to be rather straightforward, but
> the dynamic allocation part seems to be anything but.

retrofitting a new idea is usually a high level of effort.

> 
> I also wonder how useful this queue manager proposal would be in light
> of Mellanox's subfunction proposal. If people just start to create
> many small netdevs (albeit at high cost which people may argue
> against) consisting of just an rx/tx queue pair, then the queue
> manager dynamic allocation proposal would not be as useful. We could
> just use one of these netdevs to bind to in the AF_XDP case and always
> just specify queue 0. But one can argue that queue management is
> needed even for the subfunction approach, but then it would be at a
> much lower level than what I proposed. What is your take on this?
> Still worth pursuing in some form or another? If yes, then we really
> need to come up with an easy way of supporting this in current
> drivers. It is not going to fly otherwise, IMHO.
> 

I need to find some time to take a deep dive on the subfunction idea. I
like the intent, but need to understand the details.

Thanks for the update.

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[Michael S. Tsirkin]

On Wed, Feb 26, 2020 at 11:00:40AM +0800, Jason Wang wrote:
> 
> On 2020/2/26 上午8:57, David Ahern wrote:
> > From: David Ahern <dahern@digitalocean.com>
> > 
> > virtio_net currently requires extra queues to install an XDP program,
> > with the rule being twice as many queues as vcpus. From a host
> > perspective this means the VM needs to have 2*vcpus vhost threads
> > for each guest NIC for which XDP is to be allowed. For example, a
> > 16 vcpu VM with 2 tap devices needs 64 vhost threads.
> > 
> > The extra queues are only needed in case an XDP program wants to
> > return XDP_TX. XDP_PASS, XDP_DROP and XDP_REDIRECT do not need
> > additional queues. Relax the queue requirement and allow XDP
> > functionality based on resources. If an XDP program is loaded and
> > there are insufficient queues, then return a warning to the user
> > and if a program returns XDP_TX just drop the packet. This allows
> > the use of the rest of the XDP functionality to work without
> > putting an unreasonable burden on the host.
> > 
> > Cc: Jason Wang <jasowang@redhat.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: David Ahern <dahern@digitalocean.com>
> > ---
> >   drivers/net/virtio_net.c | 14 ++++++++++----
> >   1 file changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 2fe7a3188282..2f4c5b2e674d 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -190,6 +190,8 @@ struct virtnet_info {
> >   	/* # of XDP queue pairs currently used by the driver */
> >   	u16 xdp_queue_pairs;
> > +	bool can_do_xdp_tx;
> > +
> >   	/* I like... big packets and I cannot lie! */
> >   	bool big_packets;
> > @@ -697,6 +699,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
> >   			len = xdp.data_end - xdp.data;
> >   			break;
> >   		case XDP_TX:
> > +			if (!vi->can_do_xdp_tx)
> > +				goto err_xdp;
> 
> 
> I wonder if using spinlock to synchronize XDP_TX is better than dropping
> here?
> 
> Thanks

I think it's less a problem with locking, and more a problem
with queue being potentially full and XDP being unable to
transmit.

From that POV just sharing the queue would already be better than just
an uncondiitonal drop, however I think this is not what XDP users came
to expect. So at this point, partitioning the queue might be reasonable.
When XDP attaches we could block until queue is mostly empty. However,
how exactly to partition the queue remains open.  Maybe it's reasonable
to limit number of RX buffers to achieve balance.

> 
> >   			stats->xdp_tx++;
> >   			xdpf = convert_to_xdp_frame(&xdp);
> >   			if (unlikely(!xdpf))
> > @@ -870,6 +874,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> >   			}
> >   			break;
> >   		case XDP_TX:
> > +			if (!vi->can_do_xdp_tx)
> > +				goto err_xdp;
> >   			stats->xdp_tx++;
> >   			xdpf = convert_to_xdp_frame(&xdp);
> >   			if (unlikely(!xdpf))
> > @@ -2435,10 +2441,10 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> >   	/* XDP requires extra queues for XDP_TX */
> >   	if (curr_qp + xdp_qp > vi->max_queue_pairs) {
> > -		NL_SET_ERR_MSG_MOD(extack, "Too few free TX rings available");
> > -		netdev_warn(dev, "request %i queues but max is %i\n",
> > -			    curr_qp + xdp_qp, vi->max_queue_pairs);
> > -		return -ENOMEM;
> > +		NL_SET_ERR_MSG_MOD(extack, "Too few free TX rings available; XDP_TX will not be allowed");
> > +		vi->can_do_xdp_tx = false;
> > +	} else {
> > +		vi->can_do_xdp_tx = true;
> >   	}
> >   	old_prog = rtnl_dereference(vi->rq[0].xdp_prog);

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[Jason Wang]

----- Original Message -----
> On Wed, Feb 26, 2020 at 11:00:40AM +0800, Jason Wang wrote:
> > 
> > On 2020/2/26 上午8:57, David Ahern wrote:
> > > From: David Ahern <dahern@digitalocean.com>
> > > 
> > > virtio_net currently requires extra queues to install an XDP program,
> > > with the rule being twice as many queues as vcpus. From a host
> > > perspective this means the VM needs to have 2*vcpus vhost threads
> > > for each guest NIC for which XDP is to be allowed. For example, a
> > > 16 vcpu VM with 2 tap devices needs 64 vhost threads.
> > > 
> > > The extra queues are only needed in case an XDP program wants to
> > > return XDP_TX. XDP_PASS, XDP_DROP and XDP_REDIRECT do not need
> > > additional queues. Relax the queue requirement and allow XDP
> > > functionality based on resources. If an XDP program is loaded and
> > > there are insufficient queues, then return a warning to the user
> > > and if a program returns XDP_TX just drop the packet. This allows
> > > the use of the rest of the XDP functionality to work without
> > > putting an unreasonable burden on the host.
> > > 
> > > Cc: Jason Wang <jasowang@redhat.com>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: David Ahern <dahern@digitalocean.com>
> > > ---
> > >   drivers/net/virtio_net.c | 14 ++++++++++----
> > >   1 file changed, 10 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 2fe7a3188282..2f4c5b2e674d 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -190,6 +190,8 @@ struct virtnet_info {
> > >   	/* # of XDP queue pairs currently used by the driver */
> > >   	u16 xdp_queue_pairs;
> > > +	bool can_do_xdp_tx;
> > > +
> > >   	/* I like... big packets and I cannot lie! */
> > >   	bool big_packets;
> > > @@ -697,6 +699,8 @@ static struct sk_buff *receive_small(struct
> > > net_device *dev,
> > >   			len = xdp.data_end - xdp.data;
> > >   			break;
> > >   		case XDP_TX:
> > > +			if (!vi->can_do_xdp_tx)
> > > +				goto err_xdp;
> > 
> > 
> > I wonder if using spinlock to synchronize XDP_TX is better than dropping
> > here?
> > 
> > Thanks
> 
> I think it's less a problem with locking, and more a problem
> with queue being potentially full and XDP being unable to
> transmit.

I'm not sure we need care about this. Even XDP_TX with dedicated queue
can meet this. And XDP generic work like this.

> 
> From that POV just sharing the queue would already be better than just
> an uncondiitonal drop, however I think this is not what XDP users came
> to expect. So at this point, partitioning the queue might be reasonable.
> When XDP attaches we could block until queue is mostly empty.

This mean XDP_TX have a higher priority which I'm not sure is good.

> However,
> how exactly to partition the queue remains open.

It would be not easy unless we have support from virtio layer.


> Maybe it's reasonable
> to limit number of RX buffers to achieve balance.
>

If I understand this correctly, this can only help to throttle
XDP_TX. But we may have XDP_REDIRECT ...

So consider either dropping or sharing is much better than not enable
XDP, we may start from them.

Thanks

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[Michael S. Tsirkin]

On Wed, Feb 26, 2020 at 02:28:10AM -0500, Jason Wang wrote:
> 
> 
> ----- Original Message -----
> > On Wed, Feb 26, 2020 at 11:00:40AM +0800, Jason Wang wrote:
> > > 
> > > On 2020/2/26 上午8:57, David Ahern wrote:
> > > > From: David Ahern <dahern@digitalocean.com>
> > > > 
> > > > virtio_net currently requires extra queues to install an XDP program,
> > > > with the rule being twice as many queues as vcpus. From a host
> > > > perspective this means the VM needs to have 2*vcpus vhost threads
> > > > for each guest NIC for which XDP is to be allowed. For example, a
> > > > 16 vcpu VM with 2 tap devices needs 64 vhost threads.
> > > > 
> > > > The extra queues are only needed in case an XDP program wants to
> > > > return XDP_TX. XDP_PASS, XDP_DROP and XDP_REDIRECT do not need
> > > > additional queues. Relax the queue requirement and allow XDP
> > > > functionality based on resources. If an XDP program is loaded and
> > > > there are insufficient queues, then return a warning to the user
> > > > and if a program returns XDP_TX just drop the packet. This allows
> > > > the use of the rest of the XDP functionality to work without
> > > > putting an unreasonable burden on the host.
> > > > 
> > > > Cc: Jason Wang <jasowang@redhat.com>
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: David Ahern <dahern@digitalocean.com>
> > > > ---
> > > >   drivers/net/virtio_net.c | 14 ++++++++++----
> > > >   1 file changed, 10 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index 2fe7a3188282..2f4c5b2e674d 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -190,6 +190,8 @@ struct virtnet_info {
> > > >   	/* # of XDP queue pairs currently used by the driver */
> > > >   	u16 xdp_queue_pairs;
> > > > +	bool can_do_xdp_tx;
> > > > +
> > > >   	/* I like... big packets and I cannot lie! */
> > > >   	bool big_packets;
> > > > @@ -697,6 +699,8 @@ static struct sk_buff *receive_small(struct
> > > > net_device *dev,
> > > >   			len = xdp.data_end - xdp.data;
> > > >   			break;
> > > >   		case XDP_TX:
> > > > +			if (!vi->can_do_xdp_tx)
> > > > +				goto err_xdp;
> > > 
> > > 
> > > I wonder if using spinlock to synchronize XDP_TX is better than dropping
> > > here?
> > > 
> > > Thanks
> > 
> > I think it's less a problem with locking, and more a problem
> > with queue being potentially full and XDP being unable to
> > transmit.
> 
> I'm not sure we need care about this. Even XDP_TX with dedicated queue
> can meet this. And XDP generic work like this.

Well with rx queue matching tx queue depth, this might work out well.

> > 
> > From that POV just sharing the queue would already be better than just
> > an uncondiitonal drop, however I think this is not what XDP users came
> > to expect. So at this point, partitioning the queue might be reasonable.
> > When XDP attaches we could block until queue is mostly empty.
> 
> This mean XDP_TX have a higher priority which I'm not sure is good.
> 
> > However,
> > how exactly to partition the queue remains open.
> 
> It would be not easy unless we have support from virtio layer.
> 
> 
> > Maybe it's reasonable
> > to limit number of RX buffers to achieve balance.
> >
> 
> If I understand this correctly, this can only help to throttle
> XDP_TX. But we may have XDP_REDIRECT ...
> 
> So consider either dropping or sharing is much better than not enable
> XDP, we may start from them.
> 
> Thanks

Yea, sharing is at least simple.

Going forward, with the new proposal for RSS recently accepted by virtio TC,
maybe it makes sense to use that in Linux.
Driver then gets full control over which RX queues are in use,
and so something like ethtool can then be used to
direct which queues are for which purpose.

Does anyone have cycles for the above?
Cc Yuri who added this to the spec.

-- 
MST

Re: [PATCH RFC net-next] virtio_net: Relax queue requirement for using XDP

[Michael S. Tsirkin]

On Tue, Feb 25, 2020 at 05:57:44PM -0700, David Ahern wrote:
> From: David Ahern <dahern@digitalocean.com>
> 
> virtio_net currently requires extra queues to install an XDP program,
> with the rule being twice as many queues as vcpus. From a host
> perspective this means the VM needs to have 2*vcpus vhost threads
> for each guest NIC for which XDP is to be allowed. For example, a
> 16 vcpu VM with 2 tap devices needs 64 vhost threads.
> 
> The extra queues are only needed in case an XDP program wants to
> return XDP_TX. XDP_PASS, XDP_DROP and XDP_REDIRECT do not need
> additional queues. Relax the queue requirement and allow XDP
> functionality based on resources. If an XDP program is loaded and
> there are insufficient queues, then return a warning to the user
> and if a program returns XDP_TX just drop the packet. This allows
> the use of the rest of the XDP functionality to work without
> putting an unreasonable burden on the host.
> 
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: David Ahern <dahern@digitalocean.com>


It isn't particularly easy for userspace to detect packets
are dropped. If there's a need for a limited XDP with
limited resources, IMHO it's better for userspace to
declare this to the driver.


> ---
>  drivers/net/virtio_net.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 2fe7a3188282..2f4c5b2e674d 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -190,6 +190,8 @@ struct virtnet_info {
>  	/* # of XDP queue pairs currently used by the driver */
>  	u16 xdp_queue_pairs;
>  
> +	bool can_do_xdp_tx;
> +
>  	/* I like... big packets and I cannot lie! */
>  	bool big_packets;
>  
> @@ -697,6 +699,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
>  			len = xdp.data_end - xdp.data;
>  			break;
>  		case XDP_TX:
> +			if (!vi->can_do_xdp_tx)
> +				goto err_xdp;
>  			stats->xdp_tx++;
>  			xdpf = convert_to_xdp_frame(&xdp);
>  			if (unlikely(!xdpf))
> @@ -870,6 +874,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>  			}
>  			break;
>  		case XDP_TX:
> +			if (!vi->can_do_xdp_tx)
> +				goto err_xdp;
>  			stats->xdp_tx++;
>  			xdpf = convert_to_xdp_frame(&xdp);
>  			if (unlikely(!xdpf))
> @@ -2435,10 +2441,10 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>  
>  	/* XDP requires extra queues for XDP_TX */
>  	if (curr_qp + xdp_qp > vi->max_queue_pairs) {
> -		NL_SET_ERR_MSG_MOD(extack, "Too few free TX rings available");
> -		netdev_warn(dev, "request %i queues but max is %i\n",
> -			    curr_qp + xdp_qp, vi->max_queue_pairs);
> -		return -ENOMEM;
> +		NL_SET_ERR_MSG_MOD(extack, "Too few free TX rings available; XDP_TX will not be allowed");
> +		vi->can_do_xdp_tx = false;
> +	} else {
> +		vi->can_do_xdp_tx = true;
>  	}
>  
>  	old_prog = rtnl_dereference(vi->rq[0].xdp_prog);
> -- 
> 2.17.1