Re: [ovs-dev] [PATCH 1/3] dpif-netdev: only poll enabled vhost queues

[David Marchand]

On Fri, Apr 12, 2019 at 2:12 PM Ilya Maximets 
wrote:

> On 11.04.2019 17:00, David Marchand wrote:
> > We currently poll all available queues based on the max queue count
> > exchanged with the vhost peer and rely on the vhost library in DPDK to
> > check the vring status beneath.
> > This can lead to some overhead when we have a lot of unused queues.
> >
> > To enhance the situation, we can skip the disabled queues.
> > On rxq notifications, we make use of the netdev's change_seq number so
> > that the pmd thread main loop can cache the queue state periodically.
> >
> > $ ovs-appctl dpif-netdev/pmd-rxq-show
> > pmd thread numa_id 0 core_id 1:
> >   isolated : true
> >   port: dpdk0 queue-id:  0  pmd usage:  0 %  polling: enabled
> > pmd thread numa_id 0 core_id 2:
> >   isolated : true
> >   port: vhost1queue-id:  0  pmd usage:  0 %  polling:
> disabled
> >   port: vhost3queue-id:  0  pmd usage:  0 %  polling:
> disabled
> > pmd thread numa_id 0 core_id 15:
> >   isolated : true
> >   port: dpdk1 queue-id:  0  pmd usage:  0 %  polling: enabled
> > pmd thread numa_id 0 core_id 16:
> >   isolated : true
> >   port: vhost0queue-id:  0  pmd usage:  0 %  polling:
> disabled
> >   port: vhost2queue-id:  0  pmd usage:  0 %  polling:
> disabled
> >
> > $ while true; do
> >   ovs-appctl dpif-netdev/pmd-rxq-show |awk '
> >   /polling: / {
> > tot++;
> > if ($NF == "enabled") {
> >   en++;
> > }
> >   }
> >   END {
> > print "total: " tot ", enabled: " en
> >   }'
> >   sleep 1
> > done
> >
> > total: 6, enabled: 2
> > total: 6, enabled: 2
> > ...
> >
> >  # Started vm, virtio devices are bound to kernel driver which enables
> >  # F_MQ + all queue pairs
> > total: 6, enabled: 2
> > total: 66, enabled: 66
> > ...
> >
> >  # Unbound vhost0 and vhost1 from the kernel driver
> > total: 66, enabled: 66
> > total: 66, enabled: 34
> > ...
> >
> >  # Configured kernel bound devices to use only 1 queue pair
> > total: 66, enabled: 34
> > total: 66, enabled: 19
> > total: 66, enabled: 4
> > ...
> >
> >  # While rebooting the vm
> > total: 66, enabled: 4
> > total: 66, enabled: 2
> > ...
> > total: 66, enabled: 66
> > ...
> >
> >  # After shutting down the vm
> > total: 66, enabled: 66
> > total: 66, enabled: 2
> >
> > Signed-off-by: David Marchand 
> > ---
>
> Hi.
> One important issue:
> You need to enable queue #0 in case there was no state changes.
> We're doing this for Tx queues in 'dpdk_vhost_reconfigure_helper'.
> This is because if F_PROTOCOL_FEATURES not negotiated, vrings are
> created in enabled state. Required to support older QEMU or legacy
> drivers.
>

Ah ok, that explains this txq0 special treatment.
Thanks.


> I'm working on DPDK patch to trigger 'vring_state_changed' in this
> case, however it could take long or never be accepted to LTS.
> I'd also like not to check for negotiated features in OVS code.
> I think this kind of stuff should be hidden by vhost library from
> the application.
>

Ok, we will have to keep this workaround anyway.



>
> More comments inline.
>
> Also, you might want to add Ian to CC list while sending userspace
> datapath related patches.
>

I dropped the previous Cc: list by mistake.
Sure will keep him Cc.



> >  lib/dpif-netdev.c | 27 
> >  lib/netdev-dpdk.c | 71
> +--
> >  lib/netdev-provider.h |  5 
> >  lib/netdev.c  | 10 
> >  lib/netdev.h  |  1 +
> >  5 files changed, 101 insertions(+), 13 deletions(-)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 4d6d0c3..c95612f 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -591,6 +591,8 @@ struct polled_queue {
> >  struct dp_netdev_rxq *rxq;
> >  odp_port_t port_no;
> >  bool emc_enabled;
> > +bool enabled;
> > +uint64_t change_seq;
> >  };
> >
> >  /* Contained by struct dp_netdev_pmd_thread's 'poll_list' member. */
> > @@ -1171,6 +1173,9 @@ pmd_info_show_rxq(struct ds *reply, struct
> dp_netdev_pmd_thread *pmd)
> >  } else {
> >  ds_put_format(reply, "%s", "NOT AVAIL");
> >  }
> > +ds_put_format(reply, "  polling: %s",
> > +  (netdev_rxq_enabled(list[i].rxq->rx))
> > +  ? "enabled" : "disabled");
> >  ds_put_cstr(reply, "\n");
> >  }
> >  ovs_mutex_unlock(&pmd->port_mutex);
> > @@ -5198,6 +5203,11 @@ dpif_netdev_run(struct dpif *dpif)
> >  }
> >
> >  for (i = 0; i < port->n_rxq; i++) {
> > +
> > +if (!netdev_rxq_enabled(port->rxqs[i].rx)) {
> > +continue;
> > +}
> > +
> >  if (dp_netdev_process_rxq_port(non_pmd,
> > &port->rxqs[i],
> >  

Re: [ovs-dev] [PATCH 1/3] dpif-netdev: only poll enabled vhost queues

[David Marchand]

On Fri, Apr 12, 2019 at 12:00 PM Ilya Maximets 
wrote:

> On 11.04.2019 17:13, David Marchand wrote:
> >
> >
> > On Thu, Apr 11, 2019 at 4:02 PM David Marchand <
> david.march...@redhat.com > wrote:
> >
> > We currently poll all available queues based on the max queue count
> > exchanged with the vhost peer and rely on the vhost library in DPDK
> to
> > check the vring status beneath.
> > This can lead to some overhead when we have a lot of unused queues.
> >
> > To enhance the situation, we can skip the disabled queues.
> > On rxq notifications, we make use of the netdev's change_seq number
> so
> > that the pmd thread main loop can cache the queue state periodically.
> >
> > $ ovs-appctl dpif-netdev/pmd-rxq-show
> > pmd thread numa_id 0 core_id 1:
> >   isolated : true
> >   port: dpdk0 queue-id:  0  pmd usage:  0 %  polling:
> enabled
> > pmd thread numa_id 0 core_id 2:
> >   isolated : true
> >   port: vhost1queue-id:  0  pmd usage:  0 %  polling:
> disabled
> >   port: vhost3queue-id:  0  pmd usage:  0 %  polling:
> disabled
> > pmd thread numa_id 0 core_id 15:
> >   isolated : true
> >   port: dpdk1 queue-id:  0  pmd usage:  0 %  polling:
> enabled
> > pmd thread numa_id 0 core_id 16:
> >   isolated : true
> >   port: vhost0queue-id:  0  pmd usage:  0 %  polling:
> disabled
> >   port: vhost2queue-id:  0  pmd usage:  0 %  polling:
> disabled
> >
> >
> > This change broke the tests, since it matches the exact command output.
> > Is the output format something we must maintain ?
>
> I don't think so.
> You just need to fix tests to match with the new output format.
> However, in general, the output is already overloaded. Maybe it's
> worth to print polling state only if it's disabled?
> This should also save you from changing most of the tests.
>

Well, either updating the test or doing this is fine for me.
I will go with your suggestion.


-- 
David Marchand
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/3] dpif-netdev: only poll enabled vhost queues

[Ilya Maximets]

On 11.04.2019 17:00, David Marchand wrote:
> We currently poll all available queues based on the max queue count
> exchanged with the vhost peer and rely on the vhost library in DPDK to
> check the vring status beneath.
> This can lead to some overhead when we have a lot of unused queues.
> 
> To enhance the situation, we can skip the disabled queues.
> On rxq notifications, we make use of the netdev's change_seq number so
> that the pmd thread main loop can cache the queue state periodically.
> 
> $ ovs-appctl dpif-netdev/pmd-rxq-show
> pmd thread numa_id 0 core_id 1:
>   isolated : true
>   port: dpdk0 queue-id:  0  pmd usage:  0 %  polling: enabled
> pmd thread numa_id 0 core_id 2:
>   isolated : true
>   port: vhost1queue-id:  0  pmd usage:  0 %  polling: disabled
>   port: vhost3queue-id:  0  pmd usage:  0 %  polling: disabled
> pmd thread numa_id 0 core_id 15:
>   isolated : true
>   port: dpdk1 queue-id:  0  pmd usage:  0 %  polling: enabled
> pmd thread numa_id 0 core_id 16:
>   isolated : true
>   port: vhost0queue-id:  0  pmd usage:  0 %  polling: disabled
>   port: vhost2queue-id:  0  pmd usage:  0 %  polling: disabled
> 
> $ while true; do
>   ovs-appctl dpif-netdev/pmd-rxq-show |awk '
>   /polling: / {
> tot++;
> if ($NF == "enabled") {
>   en++;
> }
>   }
>   END {
> print "total: " tot ", enabled: " en
>   }'
>   sleep 1
> done
> 
> total: 6, enabled: 2
> total: 6, enabled: 2
> ...
> 
>  # Started vm, virtio devices are bound to kernel driver which enables
>  # F_MQ + all queue pairs
> total: 6, enabled: 2
> total: 66, enabled: 66
> ...
> 
>  # Unbound vhost0 and vhost1 from the kernel driver
> total: 66, enabled: 66
> total: 66, enabled: 34
> ...
> 
>  # Configured kernel bound devices to use only 1 queue pair
> total: 66, enabled: 34
> total: 66, enabled: 19
> total: 66, enabled: 4
> ...
> 
>  # While rebooting the vm
> total: 66, enabled: 4
> total: 66, enabled: 2
> ...
> total: 66, enabled: 66
> ...
> 
>  # After shutting down the vm
> total: 66, enabled: 66
> total: 66, enabled: 2
> 
> Signed-off-by: David Marchand 
> ---

Hi.
One important issue:
You need to enable queue #0 in case there was no state changes.
We're doing this for Tx queues in 'dpdk_vhost_reconfigure_helper'.
This is because if F_PROTOCOL_FEATURES not negotiated, vrings are
created in enabled state. Required to support older QEMU or legacy
drivers.

I'm working on DPDK patch to trigger 'vring_state_changed' in this
case, however it could take long or never be accepted to LTS.
I'd also like not to check for negotiated features in OVS code.
I think this kind of stuff should be hidden by vhost library from
the application.

More comments inline.

Also, you might want to add Ian to CC list while sending userspace
datapath related patches.

>  lib/dpif-netdev.c | 27 
>  lib/netdev-dpdk.c | 71 
> +--
>  lib/netdev-provider.h |  5 
>  lib/netdev.c  | 10 
>  lib/netdev.h  |  1 +
>  5 files changed, 101 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 4d6d0c3..c95612f 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -591,6 +591,8 @@ struct polled_queue {
>  struct dp_netdev_rxq *rxq;
>  odp_port_t port_no;
>  bool emc_enabled;
> +bool enabled;
> +uint64_t change_seq;
>  };
>  
>  /* Contained by struct dp_netdev_pmd_thread's 'poll_list' member. */
> @@ -1171,6 +1173,9 @@ pmd_info_show_rxq(struct ds *reply, struct 
> dp_netdev_pmd_thread *pmd)
>  } else {
>  ds_put_format(reply, "%s", "NOT AVAIL");
>  }
> +ds_put_format(reply, "  polling: %s",
> +  (netdev_rxq_enabled(list[i].rxq->rx))
> +  ? "enabled" : "disabled");
>  ds_put_cstr(reply, "\n");
>  }
>  ovs_mutex_unlock(&pmd->port_mutex);
> @@ -5198,6 +5203,11 @@ dpif_netdev_run(struct dpif *dpif)
>  }
>  
>  for (i = 0; i < port->n_rxq; i++) {
> +
> +if (!netdev_rxq_enabled(port->rxqs[i].rx)) {
> +continue;
> +}
> +
>  if (dp_netdev_process_rxq_port(non_pmd,
> &port->rxqs[i],
> port->port_no)) {
> @@ -5371,6 +5381,9 @@ pmd_load_queues_and_ports(struct dp_netdev_pmd_thread 
> *pmd,
>  poll_list[i].rxq = poll->rxq;
>  poll_list[i].port_no = poll->rxq->port->port_no;
>  poll_list[i].emc_enabled = poll->rxq->port->emc_enabled;
> +poll_list[i].enabled = netdev_rxq_enabled(poll->rxq->rx);
> +poll_list[i].change_seq =
> + netdev_get_change_seq(poll->rxq->port->netdev);
>  i++;
>  }
>  
> @@ -5436,6 +54

Re: [ovs-dev] [PATCH 1/3] dpif-netdev: only poll enabled vhost queues

[Ilya Maximets]

On 11.04.2019 17:13, David Marchand wrote:
> 
> 
> On Thu, Apr 11, 2019 at 4:02 PM David Marchand  > wrote:
> 
> We currently poll all available queues based on the max queue count
> exchanged with the vhost peer and rely on the vhost library in DPDK to
> check the vring status beneath.
> This can lead to some overhead when we have a lot of unused queues.
> 
> To enhance the situation, we can skip the disabled queues.
> On rxq notifications, we make use of the netdev's change_seq number so
> that the pmd thread main loop can cache the queue state periodically.
> 
> $ ovs-appctl dpif-netdev/pmd-rxq-show
> pmd thread numa_id 0 core_id 1:
>   isolated : true
>   port: dpdk0             queue-id:  0  pmd usage:  0 %  polling: enabled
> pmd thread numa_id 0 core_id 2:
>   isolated : true
>   port: vhost1            queue-id:  0  pmd usage:  0 %  polling: disabled
>   port: vhost3            queue-id:  0  pmd usage:  0 %  polling: disabled
> pmd thread numa_id 0 core_id 15:
>   isolated : true
>   port: dpdk1             queue-id:  0  pmd usage:  0 %  polling: enabled
> pmd thread numa_id 0 core_id 16:
>   isolated : true
>   port: vhost0            queue-id:  0  pmd usage:  0 %  polling: disabled
>   port: vhost2            queue-id:  0  pmd usage:  0 %  polling: disabled
> 
> 
> This change broke the tests, since it matches the exact command output.
> Is the output format something we must maintain ?

I don't think so.
You just need to fix tests to match with the new output format.
However, in general, the output is already overloaded. Maybe it's
worth to print polling state only if it's disabled?
This should also save you from changing most of the tests.

> 
> 
> -- 
> David Marchand
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/3] dpif-netdev: only poll enabled vhost queues

[David Marchand]

On Thu, Apr 11, 2019 at 4:02 PM David Marchand 
wrote:

> We currently poll all available queues based on the max queue count
> exchanged with the vhost peer and rely on the vhost library in DPDK to
> check the vring status beneath.
> This can lead to some overhead when we have a lot of unused queues.
>
> To enhance the situation, we can skip the disabled queues.
> On rxq notifications, we make use of the netdev's change_seq number so
> that the pmd thread main loop can cache the queue state periodically.
>
> $ ovs-appctl dpif-netdev/pmd-rxq-show
> pmd thread numa_id 0 core_id 1:
>   isolated : true
>   port: dpdk0 queue-id:  0  pmd usage:  0 %  polling: enabled
> pmd thread numa_id 0 core_id 2:
>   isolated : true
>   port: vhost1queue-id:  0  pmd usage:  0 %  polling: disabled
>   port: vhost3queue-id:  0  pmd usage:  0 %  polling: disabled
> pmd thread numa_id 0 core_id 15:
>   isolated : true
>   port: dpdk1 queue-id:  0  pmd usage:  0 %  polling: enabled
> pmd thread numa_id 0 core_id 16:
>   isolated : true
>   port: vhost0queue-id:  0  pmd usage:  0 %  polling: disabled
>   port: vhost2queue-id:  0  pmd usage:  0 %  polling: disabled
>

This change broke the tests, since it matches the exact command output.
Is the output format something we must maintain ?


-- 
David Marchand
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev