冉 jiang
2019-Jul-19 15:31 UTC
[PATCH] virtio-net: parameterize min ring num_free for virtio receive
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 at hotmail.com> >>>>>> 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 at 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??? 400kJiang
冉 jiang
2019-Jul-23 12:05 UTC
[PATCH] virtio-net: parameterize min ring num_free for virtio receive
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 at hotmail.com> >>>>>>>> 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 at 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
Reasonably Related Threads
- [PATCH] virtio-net: parameterize min ring num_free for virtio receive
- [PATCH] virtio-net: parameterize min ring num_free for virtio receive
- [PATCH] virtio-net: parameterize min ring num_free for virtio receive
- [PATCH] virtio-net: parameterize min ring num_free for virtio receive
- [PATCH] virtio-net: parameterize min ring num_free for virtio receive