[ovs-dev] netdev-dpdk: add a command to dump vhost-user info.

Flavio Leitner fbl at redhat.com
Tue Nov 7 22:09:24 UTC 2017


On Tue, 07 Nov 2017 17:29:14 +0300
Ilya Maximets <i.maximets at samsung.com> wrote:

> Thanks for the patch. One general comment:
> What do you think about exposing this information/part of this
> information via get_status() call for vhost netdevs like it done
> for physical ports?

Well, I have the same concern as exposing the new command in the
howto. DPDK has a history of ABI/API instabilities. We should be
very cautious to expose things to OVS users to not break their
environments in the future.


> At least this will provide invariant way to check "numa_id" for
> any dpdk interface.
> 
> Implementation comments inline.
> 
> Best regards, Ilya Maximets.
> 
> On 06.11.2017 17:20, Flavio Leitner wrote:
> > Add a command to dump vhost-user's information.
> > 
> > Signed-off-by: Flavio Leitner <fbl at redhat.com>
> > ---
> >  lib/netdev-dpdk.c          | 124 ++++++++++++++++++++++++++++++++++++++++++++-
> >  vswitchd/ovs-vswitchd.8.in |   4 ++
> >  2 files changed, 126 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> > index 1e9d78f58..b60a26d1f 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -974,20 +974,140 @@ dpdk_dev_parse_name(const char dev_name[], const char prefix[],
> >      }
> >  }
> >  
> > +static void
> > +netdev_dpdk_vhost_show__(struct ds *ds, struct netdev *netdev)
> > +{
> > +    struct netdev_dpdk *dpdk_dev = netdev_dpdk_cast(netdev);  
> 
> Common comment for implementation:
> It'll be nice to follow variables naming convention applied in
> d46285a2206f ("netdev-dpdk: Consistent variable naming.").

OK.


> I also posted the patch-set to fix netdev_dpdk_set_admin_state()
> (I guess, that function was a prototype for yours.) and document the
> convention more formally.

Thanks!

> > +    bool client_mode = dpdk_dev->vhost_driver_flags & RTE_VHOST_USER_CLIENT;
> > +    struct rte_vhost_vring vring;
> > +    char socket_name[PATH_MAX];
> > +    uint64_t features;
> > +    uint16_t mtu;
> > +    int16_t vring_num;  
> 
> Why 'int16_t' ? 'rte_vhost_get_vring_num()' returns 'uint16_t'.

Typo, will fix it.

 
> > +    int numa;
> > +    int vid;
> > +    int i;
> > +
> > +    vid = netdev_dpdk_get_vid(dpdk_dev);
> > +    ds_put_format(ds, "vhost-user port: %s\n", netdev->name);  
> 
> netdev_get_name(netdev) ?

Absolutely, will fix that too.

 
> > +    ds_put_format(ds, "\tMode: %s\n", client_mode ? "client" : "server");
> > +    if (!rte_vhost_get_ifname(vid, socket_name, PATH_MAX)) {  
> 
> 1. This should be done after the 'vid' checking to avoid passing '-1' to DPDK
>    and reading random memory.

Indeed.

> 2. Can we just use 'dev->vhost_id' here?

Well, that's OVS side and we can get that from ovs-vsctl show
already, so I thought it would be nice to have the DPDK side
instead.


> > +        ds_put_format(ds, "\tSocket: %s\n", socket_name);
> > +    }
> > +
> > +    if (vid == -1) {
> > +        ds_put_cstr(ds, "\tStatus: Disconnected\n");
> > +        return;
> > +    } else {
> > +        ds_put_cstr(ds, "\tStatus: Connected\n");
> > +    }
> > +
> > +    if (!rte_vhost_get_negotiated_features(vid, &features)) {
> > +        ds_put_format(ds, "\tNegotiated features: 0x%lx\n", features);  
> 
> How about "\tNegotiated features: 0x%016"PRIx64"\n" ?

OK

> 
> > +    }
> > +
> > +    if (!rte_vhost_get_mtu(vid, &mtu)) {
> > +        ds_put_format(ds, "\tMTU: %d\n", mtu);
> > +    }
> > +
> > +    numa = rte_vhost_get_numa_node(vid);
> > +    if (numa != -1) {
> > +        ds_put_format(ds, "\tNUMA: %d\n", numa);
> > +    }
> > +
> > +    vring_num = rte_vhost_get_vring_num(vid);
> > +    if (vring_num) {
> > +        ds_put_format(ds, "\tNumber of vrings: %d\n", vring_num);
> > +    }
> > +
> > +    for (i = 0; i < vring_num; i++) {
> > +        rte_vhost_get_vhost_vring(vid, i, &vring);
> > +        ds_put_format(ds, "\tVring %d:\n", i);
> > +        ds_put_format(ds, "\t\tDescriptor length: %d\n",
> > +                      vring.desc->len);
> > +        ds_put_format(ds, "\t\tRing size: %d\n", vring.size);
> > +    }
> > +}
> > +
> > +
> > +static void
> > +netdev_dpdk_vhost_show(struct unixctl_conn *conn, int argc OVS_UNUSED,  
> 
> 'argc' is not UNUSED.

It was before, believe me :-)
will fix it

> > +                            const char *argv[], void *aux OVS_UNUSED)
> > +{
> > +    struct ds ds = DS_EMPTY_INITIALIZER;
> > +    struct netdev_dpdk *dpdk_dev;
> > +
> > +    if (argc > 1) {
> > +        struct netdev *netdev = netdev_from_name(argv[1]);
> > +        if (!netdev) {
> > +            unixctl_command_reply_error(conn, "No such port");
> > +            return;
> > +        }
> > +
> > +        if (!is_dpdk_class(netdev->netdev_class)) {
> > +            netdev_close(netdev);
> > +            unixctl_command_reply_error(conn, "Not a vhost-user port");
> > +            return;
> > +        }  
> 
> Above two cases could be combined to reduce the code size. IMHO, differentiation
> of these cases is not so important.
> Note: You can safely call netdev_close(NULL) without issues.

I have no strong opinion either way.
Will fix it.

> > +
> > +        dpdk_dev = netdev_dpdk_cast(netdev);
> > +        ovs_mutex_lock(&dpdk_dev->mutex);
> > +        if (dpdk_dev->type != DPDK_DEV_VHOST) {
> > +            ovs_mutex_unlock(&dpdk_dev->mutex);
> > +            netdev_close(netdev);
> > +            unixctl_command_reply_error(conn, "Not a vhost-user port");
> > +            return;
> > +        }
> > +
> > +        netdev_dpdk_vhost_show__(&ds, netdev);
> > +        ovs_mutex_unlock(&dpdk_dev->mutex);
> > +        netdev_close(netdev);
> > +    } else {
> > +        ovs_mutex_lock(&dpdk_mutex);
> > +        LIST_FOR_EACH (dpdk_dev, list_node, &dpdk_list) {
> > +            ovs_mutex_lock(&dpdk_dev->mutex);
> > +            if (dpdk_dev->type != DPDK_DEV_VHOST) {
> > +                ovs_mutex_unlock(&dpdk_dev->mutex);
> > +                continue;
> > +            }
> > +
> > +            netdev_dpdk_vhost_show__(&ds, dpdk_dev);  
> 
> Type mismatch. 'struct netdev_dpdk *' passed instead of 'struct netdev *'.
> Have you compiled this?

Argh, yes, in my test system which has a fix for that, but
not on my devel system.

> 
> > +            ovs_mutex_unlock(&dpdk_dev->mutex);
> > +        }
> > +        ovs_mutex_unlock(&dpdk_mutex);
> > +    }
> > +
> > +    unixctl_command_reply(conn, ds_cstr(&ds));
> > +    ds_destroy(&ds);
> > +}
> > +
> >  static int
> >  vhost_common_construct(struct netdev *netdev)
> >      OVS_REQUIRES(dpdk_mutex)
> >  {
> > +    static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> >      int socket_id = rte_lcore_to_socket_id(rte_get_master_lcore());
> >      struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> > +    int error;
> >  
> >      dev->tx_q = netdev_dpdk_alloc_txq(OVS_VHOST_MAX_QUEUE_NUM);
> >      if (!dev->tx_q) {
> >          return ENOMEM;
> >      }
> >  
> > -    return common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
> > -                            DPDK_DEV_VHOST, socket_id);
> > +    error = common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
> > +                             DPDK_DEV_VHOST, socket_id);
> > +    if (error) {
> > +        return error;
> > +    }
> > +
> > +    if (ovsthread_once_start(&once)) {
> > +        unixctl_command_register("vhostuser/show", "[port]", 0, 1,
> > +                                 netdev_dpdk_vhost_show, NULL);  
> 
> IMHO, this should be done in class_init() like for physical ports.
> There is no reason to have this appctl not defined until we have first
> vhost-user port.

The list of commands is growing and it is big already (95 commands).
IMHO there is no point in having the command always available if you
don't have ports to use.


> About naming, I think there is no need to have yet another namespace
> 'vhostuser' for appctl calls. It only confuses the users.
> Also, we already have 'netdev-dpdk' namespace which covers all the
> DPDK interface types.
> (We, actually, need to move at least 'set-admin-state' to some common
> function like 'netdev_dpdk_register()', because it works not only
> with ETH interfaces. It also works with vhostuser ports.)
> 
> How about 'netdev-dpdk/vhost-user-info' or something similar?

Initially I thought about doing that, but I figure that we might have
other interesting vhost-user commands and as there is almost no
practical impact, I decided for opening a new namespace.

No strong opinion either way, so I will update.


> This will also help to avoid documentation fragmenting.
> We'll be able to just add this command to something similar to
> 'lib/netdev-dpdk-unixctl.man' from my recent series:
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-October/340245.html

Yeah, I can add this on top of that series, no issues.

> And once again, I'm still thinking that we can add some of this information
> to 'get_status()' instead of exposing via appctl.

see my comments at the top.
Thanks for the good review, I appreciated it!
fbl

> 
> > +        ovsthread_once_done(&once);
> > +    }
> > +
> > +    return 0;
> >  }
> >  
> >  static int
> > diff --git a/vswitchd/ovs-vswitchd.8.in b/vswitchd/ovs-vswitchd.8.in
> > index c18baf6db..607f6b046 100644
> > --- a/vswitchd/ovs-vswitchd.8.in
> > +++ b/vswitchd/ovs-vswitchd.8.in
> > @@ -156,6 +156,10 @@ event on all bridges.
> >  Displays detailed information about rapid spanning tree on the \fIbridge\fR.
> >  If \fIbridge\fR is not specified, then displays detailed information about all
> >  bridges with RSTP enabled.
> > +.IP "\fBvhostuser/show\fR [\fIport\fR]"
> > +Displays detailed internal information about a specific vhost-user \fIport\fR.
> > +If \fIport\fR is not specified, then displays detailed information about all
> > +vhost-user ports.
> >  .SS "BRIDGE COMMANDS"
> >  These commands manage bridges.
> >  .IP "\fBfdb/flush\fR [\fIbridge\fR]"
> >   



-- 
Flavio


More information about the dev mailing list