On Thu, Jan 20, 2022 at 04:08:39PM +0100, Stefano Garzarella wrote:

On Fri, Jan 14, 2022 at 2:40 PM Michael S. Tsirkin [off-list ref] wrote:

quoted

On Fri, Jan 14, 2022 at 02:38:16PM +0100, Stefano Garzarella wrote:

quoted

On Fri, Jan 14, 2022 at 07:45:35AM -0500, Michael S. Tsirkin wrote:

quoted

On Fri, Jan 14, 2022 at 10:05:08AM +0100, Stefano Garzarella wrote:

quoted

In vhost_enable_notify() we enable the notifications and we read
the avail index to check if new buffers have become available in
the meantime.

We are not caching the avail index, so when the device will call
vhost_get_vq_desc(), it will find the old value in the cache and
it will read the avail index again.

It would be better to refresh the cache every time we read avail
index, so let's change vhost_enable_notify() caching the value in
`avail_idx` and compare it with `last_avail_idx` to check if there
are new buffers available.

Anyway, we don't expect a significant performance boost because
the above path is not very common, indeed vhost_enable_notify()
is often called with unlikely(), expecting that avail index has
not been updated.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

... and can in theory even hurt due to an extra memory write.
So ... performance test restults pls?

Right, could be.

I'll run some perf test with vsock, about net, do you have a test suite or
common step to follow to test it?

Thanks,
Stefano

You can use the vhost test as a unit test as well.

Thanks for the advice, I did indeed use it!

I run virtio_test (with vhost_test.ko) using 64 as batch to increase the 
chance of the path being taken. (I changed bufs=0x1000000 in 
virtio_test.c to increase the duration).

I used `perf stat` to take some numbers, running this command:

   taskset -c 2 perf stat -r 10 --log-fd 1 -- ./virtio_test --batch=64

- Linux v5.16 without the patch applied

 Performance counter stats for './virtio_test --batch=64' (10 runs):

          2,791.70 msec task-clock                #    0.996 CPUs utilized            ( +-  0.36% )
                23      context-switches          #    8.209 /sec                     ( +-  2.75% )
                 0      cpu-migrations            #    0.000 /sec
                79      page-faults               #   28.195 /sec                     ( +-  0.41% )
     7,249,926,989      cycles                    #    2.587 GHz                      ( +-  0.36% )
     7,711,999,656      instructions              #    1.06  insn per cycle           ( +-  1.08% )
     1,838,436,806      branches                  #  656.134 M/sec                    ( +-  1.44% )
         3,055,439      branch-misses             #    0.17% of all branches          ( +-  6.22% )

            2.8024 +- 0.0100 seconds time elapsed  ( +-  0.36% )

- Linux v5.16 with this patch applied

 Performance counter stats for './virtio_test --batch=64' (10 runs):

          2,753.36 msec task-clock                #    0.998 CPUs utilized            ( +-  0.20% )
                24      context-switches          #    8.699 /sec                     ( +-  2.86% )
                 0      cpu-migrations            #    0.000 /sec
                76      page-faults               #   27.545 /sec                     ( +-  0.56% )
     7,150,358,721      cycles                    #    2.592 GHz                      ( +-  0.20% )
     7,420,639,950      instructions              #    1.04  insn per cycle           ( +-  0.76% )
     1,745,759,193      branches                  #  632.730 M/sec                    ( +-  1.03% )
         3,022,508      branch-misses             #    0.17% of all branches          ( +-  3.24% )

           2.75952 +- 0.00561 seconds time elapsed  ( +-  0.20% )


The difference seems minimal with a slight improvement.

To try to stress the patch more, I modified vhost_test.ko to call 
vhost_enable_notify()/vhost_disable_notify() on every cycle when calling 
vhost_get_vq_desc():

- Linux v5.16 modified without the patch applied

 Performance counter stats for './virtio_test --batch=64' (10 runs):

          4,126.66 msec task-clock                #    1.006 CPUs utilized            ( +-  0.25% )
                28      context-switches          #    6.826 /sec                     ( +-  3.41% )
                 0      cpu-migrations            #    0.000 /sec
                85      page-faults               #   20.721 /sec                     ( +-  0.44% )
    10,716,808,883      cycles                    #    2.612 GHz                      ( +-  0.25% )
    11,804,381,462      instructions              #    1.11  insn per cycle           ( +-  0.86% )
     3,138,813,438      branches                  #  765.153 M/sec                    ( +-  1.03% )
        11,286,860      branch-misses             #    0.35% of all branches          ( +-  1.23% )

            4.1027 +- 0.0103 seconds time elapsed  ( +-  0.25% )

- Linux v5.16 modified with this patch applied

 Performance counter stats for './virtio_test --batch=64' (10 runs):

          3,953.55 msec task-clock                #    1.001 CPUs utilized            ( +-  0.33% )
                29      context-switches          #    7.345 /sec                     ( +-  2.67% )
                 0      cpu-migrations            #    0.000 /sec
                83      page-faults               #   21.021 /sec                     ( +-  0.65% )
    10,267,242,653      cycles                    #    2.600 GHz                      ( +-  0.33% )
     7,972,866,579      instructions              #    0.78  insn per cycle           ( +-  0.21% )
     1,663,770,390      branches                  #  421.377 M/sec                    ( +-  0.45% )
        16,986,093      branch-misses             #    1.02% of all branches          ( +-  0.47% )

            3.9489 +- 0.0130 seconds time elapsed  ( +-  0.33% )

In this case the difference is bigger, with a reduction in execution 
time (3.7 %) and fewer branches and instructions. It should be the 
branch `if (vq->avail_idx == vq->last_avail_idx)` in vhost_get_vq_desc() 
that is not taken.

Should I resend the patch adding some more performance information?

Thanks,
Stefano

Yea, pls do. You can just summarize it in a couple of lines.

-- 
MST