Re: [PATCH] virtio-net: parameterize min ring num_free for virtio receive

From: Michael S. Tsirkin
Date: Tue Aug 13 2019 - 06:56:03 EST

Next message: syzbot: "Re: WARNING in untrack_pfn"
Previous message: Ganapatrao Kulkarni: "Re: [PATCH v3 2/2] drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Jul 23, 2019 at 12:05:03PM +0000, å jiang wrote:
>
> On 2019/7/20 0:13, Michael S. Tsirkin wrote:
> > On Fri, Jul 19, 2019 at 03:31:29PM +0000, å jiang wrote:
> >> On 2019/7/19 22:29, Jiang wrote:
> >>> On 2019/7/19 10:36, Jason Wang wrote:
> >>>> On 2019/7/18 äå10:43, Michael S. Tsirkin wrote:
> >>>>> On Thu, Jul 18, 2019 at 10:42:47AM -0400, Michael S. Tsirkin wrote:
> >>>>>> On Thu, Jul 18, 2019 at 10:01:05PM +0800, Jason Wang wrote:
> >>>>>>> On 2019/7/18 äå9:04, Michael S. Tsirkin wrote:
> >>>>>>>> On Thu, Jul 18, 2019 at 12:55:50PM +0000, ? jiang wrote:
> >>>>>>>>> This change makes ring buffer reclaim threshold num_free
> >>>>>>>>> configurable
> >>>>>>>>> for better performance, while it's hard coded as 1/2 * queue now.
> >>>>>>>>> According to our test with qemu + dpdk, packet dropping happens
> >>>>>>>>> when
> >>>>>>>>> the guest is not able to provide free buffer in avail ring timely.
> >>>>>>>>> Smaller value of num_free does decrease the number of packet
> >>>>>>>>> dropping
> >>>>>>>>> during our test as it makes virtio_net reclaim buffer earlier.
> >>>>>>>>>
> >>>>>>>>> At least, we should leave the value changeable to user while the
> >>>>>>>>> default value as 1/2 * queue is kept.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: jiangkidd<jiangkidd@xxxxxxxxxxx>
> >>>>>>>> That would be one reason, but I suspect it's not the
> >>>>>>>> true one. If you need more buffer due to jitter
> >>>>>>>> then just increase the queue size. Would be cleaner.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> However are you sure this is the reason for
> >>>>>>>> packet drops? Do you see them dropped by dpdk
> >>>>>>>> due to lack of space in the ring? As opposed to
> >>>>>>>> by guest?
> >>>>>>>>
> >>>>>>>>
> >>>>>>> Besides those, this patch depends on the user to choose a suitable
> >>>>>>> threshold
> >>>>>>> which is not good. You need either a good value with demonstrated
> >>>>>>> numbers or
> >>>>>>> something smarter.
> >>>>>>>
> >>>>>>> Thanks
> >>>>>> I do however think that we have a problem right now: try_fill_recv can
> >>>>>> take up a long time during which net stack does not run at all.
> >>>>>> Imagine
> >>>>>> a 1K queue - we are talking 512 packets. That's exceessive.
> >>>>
> >>>> Yes, we will starve a fast host in this case.
> >>>>
> >>>>
> >>>>>> ÂÂ napi poll
> >>>>>> weight solves a similar problem, so it might make sense to cap this at
> >>>>>> napi_poll_weight.
> >>>>>>
> >>>>>> Which will allow tweaking it through a module parameter as a
> >>>>>> side effect :) Maybe just do NAPI_POLL_WEIGHT.
> >>>>> Or maybe NAPI_POLL_WEIGHT/2 like we do at half the queue ;). Please
> >>>>> experiment, measure performance and let the list know
> >>>>>
> >>>>>> Need to be careful though: queues can also be small and I don't
> >>>>>> think we
> >>>>>> want to exceed queue size / 2, or maybe queue size - napi_poll_weight.
> >>>>>> Definitely must not exceed the full queue size.
> >>>>
> >>>> Looking at intel, it uses 16 and i40e uses 32.Â It looks to me
> >>>> NAPI_POLL_WEIGHT/2 is better.
> >>>>
> >>>> Jiang, want to try that and post a new patch?
> >>>>
> >>>> Thanks
> >>>>
> >>>>
> >>>>>> --
> >>>>>> MST
> >>> We did have completed several rounds of test with setting the value to
> >>> budget (64 as the default value). It does improve a lot with pps is
> >>> below 400pps for a single stream. Let me consolidate the data and will
> >>> send it soon. Actually, we are confident that it runs out of free
> >>> buffer in avail ring when packet dropping happens with below systemtap:
> >>>
> >>> Just a snippet:
> >>>
> >>> probe module("virtio_ring").function("virtqueue_get_buf")
> >>> {
> >>> ÂÂÂ x = (@cast($_vq, "vring_virtqueue")->vring->used->idx)-
> >>> (@cast($_vq, "vring_virtqueue")->last_used_idx) ---> we use this one
> >>> to verify if the queue is full, which means guest is not able to take
> >>> buffer from the queue timely
> >>>
> >>> ÂÂÂ if (x<0 && (x+65535)<4096)
> >>> ÂÂÂ ÂÂÂ x = x+65535
> >>>
> >>> ÂÂÂ if((x==1024) && @cast($_vq, "vring_virtqueue")->vq->callback ==
> >>> callback_addr)
> >>> ÂÂÂ ÂÂÂ netrxcount[x] <<< gettimeofday_s()
> >>> }
> >>>
> >>>
> >>> probe module("virtio_ring").function("virtqueue_add_inbuf")
> >>> {
> >>> ÂÂÂ y = (@cast($vq, "vring_virtqueue")->vring->avail->idx)-
> >>> (@cast($vq, "vring_virtqueue")->vring->used->idx) ---> we use this one
> >>> to verify if we run out of free buffer in avail ring
> >>> ÂÂÂ if (y<0 && (y+65535)<4096)
> >>> ÂÂÂ ÂÂÂ y = y+65535
> >>>
> >>> ÂÂÂ if(@2=="debugon")
> >>> ÂÂÂ {
> >>> ÂÂÂ ÂÂÂ if(y==0 && @cast($vq, "vring_virtqueue")->vq->callback ==
> >>> callback_addr)
> >>> ÂÂÂ ÂÂÂ {
> >>> ÂÂÂ ÂÂÂ ÂÂÂ netrxfreecount[y] <<< gettimeofday_s()
> >>>
> >>> ÂÂÂ ÂÂÂ ÂÂÂ printf("no avail ring left seen, printing most recent 5
> >>> num free, vq: %lx, current index: %d\n", $vq, recentfreecount)
> >>> ÂÂÂ ÂÂÂ ÂÂÂ for(i=recentfreecount; i!=((recentfreecount+4) % 5);
> >>> i=((i+1) % 5))
> >>> ÂÂÂ ÂÂÂ ÂÂÂ {
> >>> ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂ printf("index: %d, num free: %d\n", i, recentfree[$vq,
> >>> i])
> >>> ÂÂÂ ÂÂÂ ÂÂÂ }
> >>>
> >>> ÂÂÂ ÂÂÂ ÂÂÂ printf("index: %d, num free: %d\n", i, recentfree[$vq, i])
> >>> ÂÂÂ ÂÂÂ ÂÂÂ //exit()
> >>> ÂÂÂ ÂÂÂ }
> >>> ÂÂÂ }
> >>> }
> >>>
> >>>
> >>> probe
> >>> module("virtio_net").statement("virtnet_receive@drivers/net/virtio_net.c:732")
> >>> {
> >>> ÂÂÂ recentfreecount++
> >>> ÂÂÂ recentfreecount = recentfreecount % 5
> >>> ÂÂÂ recentfree[$rq->vq, recentfreecount] = $rq->vq->num_free --->
> >>> record the num_free for the last 5 calls to virtnet_receive, so we can
> >>> see if lowering the bar helps.
> >>> }
> >>>
> >>>
> >>> Here is the result:
> >>>
> >>> no avail ring left seen, printing most recent 5 num free, vq:
> >>> ffff9c13c1200000, current index: 1
> >>> index: 1, num free: 561
> >>> index: 2, num free: 305
> >>> index: 3, num free: 369
> >>> index: 4, num free: 433
> >>> index: 0, num free: 497
> >>> no avail ring left seen, printing most recent 5 num free, vq:
> >>> ffff9c13c1200000, current index: 1
> >>> index: 1, num free: 543
> >>> index: 2, num free: 463
> >>> index: 3, num free: 469
> >>> index: 4, num free: 476
> >>> index: 0, num free: 479
> >>> no avail ring left seen, printing most recent 5 num free, vq:
> >>> ffff9c13c1200000, current index: 2
> >>> index: 2, num free: 555
> >>> index: 3, num free: 414
> >>> index: 4, num free: 420
> >>> index: 0, num free: 427
> >>> index: 1, num free: 491
> >>>
> >>> You can see in the last 4 calls to virtnet_receive before we run out
> >>> of free buffer and start to relaim, num_free is quite high. So if we
> >>> can do the reclaim earlier, it will certainly help.
> >>>
> >>> Meanwhile, the patch I proposed actually keeps the default value as
> >>> 1/2 * queue. So the default behavior remains and only leave the
> >>> interface to advanced users, who really understands what they are
> >>> doing. Also, the best value may vary in different environment. Do you
> >>> still think hardcoding this is better option?
> >>>
> >>>
> >>> Jiang
> >>>
> >> Here is the snippet from our test result. Test1 was done with default
> >> driver with the value of 1/2 * queue, while test2 is with my patch and
> >> min_numfree set to 64 (the default budget value). We can see average
> >> drop packets do decrease a lot in test2. Let me know if you need the
> >> full testing data.
> >>
> >> test1TimeÂÂÂ avgDropPacketsÂÂÂ test2TimeÂÂÂ avgDropPacketsÂÂÂ pps
> >>
> >>> 16:21.0ÂÂÂ 12.295ÂÂÂ 56:50.4ÂÂÂ 0ÂÂÂ 300k
> >>> 17:19.1ÂÂÂ 15.244ÂÂÂ 56:50.4ÂÂÂ 0ÂÂÂ 300k
> >>> 18:17.5ÂÂÂ 18.789ÂÂÂ 56:50.4ÂÂÂ 0ÂÂÂ 300k
> >>> 19:15.1ÂÂÂ 14.208ÂÂÂ 56:50.4ÂÂÂ 0ÂÂÂ 300k
> >>> 20:13.2ÂÂÂ 20.818ÂÂÂ 56:50.4ÂÂÂ 0.267ÂÂÂ 300k
> >>> 21:11.2ÂÂÂ 12.397ÂÂÂ 56:50.4ÂÂÂ 0ÂÂÂ 300k
> >>> 22:09.3ÂÂÂ 12.599ÂÂÂ 56:50.4ÂÂÂ 0ÂÂÂ 300k
> >>> 23:07.3ÂÂÂ 15.531ÂÂÂ 57:48.4ÂÂÂ 0ÂÂÂ 300k
> >>> 24:05.5ÂÂÂ 13.664ÂÂÂ 58:46.5ÂÂÂ 0ÂÂÂ 300k
> >>> 25:03.7ÂÂÂ 13.158ÂÂÂ 59:44.5ÂÂÂ 4.73ÂÂÂ 300k
> >>> 26:01.1ÂÂÂ 2.486ÂÂÂ 00:42.6ÂÂÂ 0ÂÂÂ 300k
> >>> 26:59.1ÂÂÂ 11.241ÂÂÂ 01:40.6ÂÂÂ 0ÂÂÂ 300k
> >>> 27:57.2ÂÂÂ 20.521ÂÂÂ 02:38.6ÂÂÂ 0ÂÂÂ 300k
> >>> 28:55.2ÂÂÂ 30.094ÂÂÂ 03:36.7ÂÂÂ 0ÂÂÂ 300k
> >>> 29:53.3ÂÂÂ 16.828ÂÂÂ 04:34.7ÂÂÂ 0.963ÂÂÂ 300k
> >>> 30:51.3ÂÂÂ 46.916ÂÂÂ 05:32.8ÂÂÂ 0ÂÂÂ 400k
> >>> 31:49.3ÂÂÂ 56.214ÂÂÂ 05:32.8ÂÂÂ 0ÂÂÂ 400k
> >>> 32:47.3ÂÂÂ 58.69ÂÂÂ 05:32.8ÂÂÂ 0ÂÂÂ 400k
> >>> 33:45.3ÂÂÂ 61.486ÂÂÂ 05:32.8ÂÂÂ 0ÂÂÂ 400k
> >>> 34:43.3ÂÂÂ 72.175ÂÂÂ 05:32.8ÂÂÂ 0.598ÂÂÂ 400k
> >>> 35:41.3ÂÂÂ 56.699ÂÂÂ 05:32.8ÂÂÂ 0ÂÂÂ 400k
> >>> 36:39.3ÂÂÂ 61.071ÂÂÂ 05:32.8ÂÂÂ 0ÂÂÂ 400k
> >>> 37:37.3ÂÂÂ 43.355ÂÂÂ 06:30.8ÂÂÂ 0ÂÂÂ 400k
> >>> 38:35.4ÂÂÂ 44.644ÂÂÂ 06:30.8ÂÂÂ 0ÂÂÂ 400k
> >>> 39:33.4ÂÂÂ 72.336ÂÂÂ 06:30.8ÂÂÂ 0ÂÂÂ 400k
> >>> 40:31.4ÂÂÂ 70.676ÂÂÂ 06:30.8ÂÂÂ 0ÂÂÂ 400k
> >>> 41:29.4ÂÂÂ 108.009ÂÂÂ 06:30.8ÂÂÂ 0ÂÂÂ 400k
> >>> 42:27.4ÂÂÂ 65.216ÂÂÂ 06:30.8ÂÂÂ 0ÂÂÂ 400k
> >>
> >> Jiang
> >
> > OK I find this surprising but I accept what you see.
> > I'm inclined not to add a tunable and just select
> > a value ourselves.
> > I'm also fine with using the napi poll module parameter
> > which will give you a bit of tunability.
>
> OK, kindly take a look if you prefer the below code change. I tested
> budget/2 and the result is almost the same as budget when pps below
> 400k, but a little better when it goes beyond 400k in my environment.
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>
> index 0d4115c9e20b..bc08be7925eb 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1331,7 +1331,7 @@ static int virtnet_receive(struct receive_queue
> *rq, int budget,
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ }
> ÂÂÂÂÂÂÂ }
>
> -ÂÂÂÂÂÂ if (rq->vq->num_free > virtqueue_get_vring_size(rq->vq) / 2) {
> +ÂÂÂÂÂÂ if (rq->vq->num_free > min((unsigned int)budget,
> virtqueue_get_vring_size(rq->vq)) / 2) {
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (!try_fill_recv(vi, rq, GFP_ATOMIC))
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ schedule_delayed_work(&vi->refill, 0);
> ÂÂÂÂÂÂÂ }
>
>
> Jiang
>

Looks good to me.
Pls post for inclusion in -net.

--
MST

Next message: syzbot: "Re: WARNING in untrack_pfn"
Previous message: Ganapatrao Kulkarni: "Re: [PATCH v3 2/2] drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]