[ovs-dev] [PATCH v2 1/3] dpif-netdev: only poll enabled vhost queues
Ilya Maximets
i.maximets at samsung.com
Wed Apr 17 10:09:59 UTC 2019
On 17.04.2019 11:35, David Marchand wrote:
> Hello Ilya,
>
>
> On Tue, Apr 16, 2019 at 3:41 PM Ilya Maximets <i.maximets at samsung.com <mailto:i.maximets at samsung.com>> wrote:
>
> Hi.
>
> One comment for the patch names. It's not a rule, but it's better
> to make the <summary> part of the subject a complete sentence.
> i.e. start with a capital letter and end with a period.
>
> Same rule is applicable to the comments in the code, but this is
> documented in coding-style.
>
>
> Another thing different from my dpdk habits.
> Need to focus when hacking ovs :-).
>
>
> On 16.04.2019 12:45, David Marchand wrote:
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 4d6d0c3..5bfa6ad 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;
>
> What do you think about renaming to 'rxq_enabled'? It seems more clear.
> Having both 'emc_enabled' and just 'enabled' is a bit confusing.
>
>
> Yes, this object is not a rxq itself, so a short enabled is confusing.
> Ok for rxq_enabled.
>
>
>
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> > index 47153dc..9ba8e67 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -424,6 +424,9 @@ struct netdev_dpdk {
> > OVSRCU_TYPE(struct ingress_policer *) ingress_policer;
> > uint32_t policer_rate;
> > uint32_t policer_burst;
> > +
> > + /* Array of vhost rxq states, see vring_state_changed */
>
> Should end with a period.
>
>
> Yes.
>
>
> > + bool *vhost_rxq_enabled;
> > );
> >
> > PADDED_MEMBERS(CACHE_LINE_SIZE,
> > @@ -1235,8 +1238,14 @@ vhost_common_construct(struct netdev *netdev)
> > int socket_id = rte_lcore_to_socket_id(rte_get_master_lcore());
> > struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> >
> > + dev->vhost_rxq_enabled = dpdk_rte_mzalloc(OVS_VHOST_MAX_QUEUE_NUM *
> > + sizeof *dev->vhost_rxq_enabled);
> > + if (!dev->vhost_rxq_enabled) {
> > + return ENOMEM;
> > + }
> > dev->tx_q = netdev_dpdk_alloc_txq(OVS_VHOST_MAX_QUEUE_NUM);
> > if (!dev->tx_q) {
> > + rte_free(dev->vhost_rxq_enabled);
> > return ENOMEM;
> > }
> >
> > @@ -1448,6 +1457,7 @@ netdev_dpdk_vhost_destruct(struct netdev *netdev)
> > dev->vhost_id = NULL;
> >
> > common_destruct(dev);
> > + rte_free(dev->vhost_rxq_enabled);
>
> Logically, 'common_destruct' should go after class-specific things.
>
>
> Indeed.
>
>
>
> >
> > ovs_mutex_unlock(&dpdk_mutex);
> >
> > @@ -2200,6 +2210,14 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
> > return 0;
> > }
> >
> > +static bool
> > +netdev_dpdk_vhost_rxq_enabled(struct netdev_rxq *rxq)
> > +{
> > + struct netdev_dpdk *dev = netdev_dpdk_cast(rxq->netdev);
> > +
> > + return dev->vhost_rxq_enabled[rxq->queue_id];
> > +}
> > +
> > static int
> > netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct dp_packet_batch *batch,
> > int *qfill)
> > @@ -3563,6 +3581,8 @@ destroy_device(int vid)
> > ovs_mutex_lock(&dev->mutex);
> > dev->vhost_reconfigured = false;
> > ovsrcu_index_set(&dev->vid, -1);
> > + memset(dev->vhost_rxq_enabled, 0,
> > + OVS_VHOST_MAX_QUEUE_NUM * sizeof *dev->vhost_rxq_enabled);
>
> We need to clear only first 'dev->up.n_rxq' queues.
>
>
> Would not hurt, but yes only clearing this part is required.
>
>
>
> > netdev_dpdk_txq_map_clear(dev);
> >
> > netdev_change_seq_changed(&dev->up);
> > @@ -3597,24 +3617,30 @@ vring_state_changed(int vid, uint16_t queue_id, int enable)
> > struct netdev_dpdk *dev;
> > bool exists = false;
> > int qid = queue_id / VIRTIO_QNUM;
> > + bool is_rx = (queue_id % VIRTIO_QNUM) == VIRTIO_TXQ;
> > char ifname[IF_NAME_SZ];
> >
> > rte_vhost_get_ifname(vid, ifname, sizeof ifname);
> >
> > - if (queue_id % VIRTIO_QNUM == VIRTIO_TXQ) {
> > - return 0;
> > - }
> > -
> > ovs_mutex_lock(&dpdk_mutex);
> > LIST_FOR_EACH (dev, list_node, &dpdk_list) {
> > ovs_mutex_lock(&dev->mutex);
> > if (nullable_string_is_equal(ifname, dev->vhost_id)) {
> > - if (enable) {
> > - dev->tx_q[qid].map = qid;
> > + if (is_rx) {
> > + bool enabled = dev->vhost_rxq_enabled[qid];
>
> This is also confusing to have 'enable' and 'enabled' in a same scope.
> What do you think about renaming 'enabled' --> 'old_state'?
>
>
> Ok.
>
>
>
> > +
> > + dev->vhost_rxq_enabled[qid] = enable != 0;
> > + if (enabled != dev->vhost_rxq_enabled[qid]) {
> > + netdev_change_seq_changed(&dev->up);
> > + }
>
>
>
>
> > diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h
> > index fb0c27e..5faae0d 100644
> > --- a/lib/netdev-provider.h
> > +++ b/lib/netdev-provider.h
> > @@ -789,6 +789,11 @@ struct netdev_class {
> > void (*rxq_destruct)(struct netdev_rxq *);
> > void (*rxq_dealloc)(struct netdev_rxq *);
> >
> > + /* A netdev can report if a queue won't get traffic and should be excluded
> > + * from polling (no callback implicitely means that the queue is enabled).
> > + */
>
> I'm suggesting following variant of this comment to be similar to other function
> descriptions here:
>
> /* Retrieves the current state of rx queue. 'False' means that queue won't
> * get traffic in a short term and could be not polled.
>
>
> I am not a reference in English language,
Me neither.
> but "could be not polled" sounds odd to me.
> Let's go with your suggestion, I don't have a better idea.
We could always fix that later if someone else will suggest a better version.
>
>
> *
> * This function may be set to null if it would always return 'True'
> * anyhow. */
>
>
> Other mentions of true are not capitalised in this header.
> Going with 'false' and 'true'.
OK.
>
>
> Thanks.
>
>
> --
> David Marchand
More information about the dev
mailing list