| Message ID | 20171106142041.10350-1-fbl@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] netdev-dpdk: add a command to dump vhost-user info. | expand |
> Add a command to dump vhost-user's information. > Thanks for the patch Flavio, I'll take a look in greater depth and test later this week. On a first pass through though, I'd like to see the command being added to the OVS DPDK vHost user how to also in documentation, just to double up on the exposure that this command is available to users. Thoughts? Ian > Signed-off-by: Flavio Leitner <fbl@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); > + 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; > + int numa; > + int vid; > + int i; > + > + vid = netdev_dpdk_get_vid(dpdk_dev); > + ds_put_format(ds, "vhost-user port: %s\n", netdev->name); > + ds_put_format(ds, "\tMode: %s\n", client_mode ? "client" : "server"); > + if (!rte_vhost_get_ifname(vid, socket_name, PATH_MAX)) { > + 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); > + } > + > + 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, > + 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; > + } > + > + 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); > + 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); > + 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]" > -- > 2.13.6 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Mon, 6 Nov 2017 20:50:01 +0000 "Stokes, Ian" <ian.stokes@intel.com> wrote: > > Add a command to dump vhost-user's information. > > > > Thanks for the patch Flavio, I'll take a look in greater depth and test later this week. On a first pass through though, I'd like to see the command being added to the OVS DPDK vHost user how to also in documentation, just to double up on the exposure that this command is available to users. Thoughts? Thanks for reviewing it! It's not meant for regular users. This is useful for debugging purposes and it exposes internal information which is subjected to change from version to version, so I wouldn't add to those regular user facing guides. fbl > > Ian > > > > Signed-off-by: Flavio Leitner <fbl@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); > > + 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; > > + int numa; > > + int vid; > > + int i; > > + > > + vid = netdev_dpdk_get_vid(dpdk_dev); > > + ds_put_format(ds, "vhost-user port: %s\n", netdev->name); > > + ds_put_format(ds, "\tMode: %s\n", client_mode ? "client" : "server"); > > + if (!rte_vhost_get_ifname(vid, socket_name, PATH_MAX)) { > > + 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); > > + } > > + > > + 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, > > + 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; > > + } > > + > > + 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); > > + 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); > > + 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]" > > -- > > 2.13.6 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > > Add a command to dump vhost-user's information. > > > > > > > Thanks for the patch Flavio, I'll take a look in greater depth and test later > this week. On a first pass through though, I'd like to see the command being > added to the OVS DPDK vHost user how to also in documentation, just to > double up on the exposure that this command is available to users. > Thoughts? > > Thanks for reviewing it! > > It's not meant for regular users. This is useful for debugging purposes and it > exposes internal information which is subjected to change from version to > version, so I wouldn't add to those regular user facing guides. > Is there somewhere it can be documented? A page with diagnostic commands? Or even in the regular docs, with a warning of 'for developer use only' or something?
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? 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@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."). 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. > + 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'. > + 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) ? > + 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. 2. Can we just use 'dev->vhost_id' here? > + 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" ? > + } > + > + 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. > + 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. > + > + 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? > + 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. 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? 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 And once again, I'm still thinking that we can add some of this information to 'get_status()' instead of exposing via appctl. > + 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]" >
On Tue, 7 Nov 2017 07:34:24 -0400 "Raymond Burkholder" <ray@oneunified.net> wrote: > > > > > > Add a command to dump vhost-user's information. > > > > > > > > > > Thanks for the patch Flavio, I'll take a look in greater depth and test > later > > this week. On a first pass through though, I'd like to see the command > being > > added to the OVS DPDK vHost user how to also in documentation, just to > > double up on the exposure that this command is available to users. > > Thoughts? > > > > Thanks for reviewing it! > > > > It's not meant for regular users. This is useful for debugging purposes > and it > > exposes internal information which is subjected to change from version to > > version, so I wouldn't add to those regular user facing guides. > > > > Is there somewhere it can be documented? A page with diagnostic commands? > Or even in the regular docs, with a warning of 'for developer use only' or > something? It's in the man-page as other ovs-appctl commands.
On Tue, 07 Nov 2017 17:29:14 +0300 Ilya Maximets <i.maximets@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@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]" > >
On Tue, Nov 07, 2017 at 07:34:24AM -0400, Raymond Burkholder wrote: > > > > > > Add a command to dump vhost-user's information. > > > > > > > > > > Thanks for the patch Flavio, I'll take a look in greater depth and test > later > > this week. On a first pass through though, I'd like to see the command > being > > added to the OVS DPDK vHost user how to also in documentation, just to > > double up on the exposure that this command is available to users. > > Thoughts? > > > > Thanks for reviewing it! > > > > It's not meant for regular users. This is useful for debugging purposes > and it > > exposes internal information which is subjected to change from version to > > version, so I wouldn't add to those regular user facing guides. > > > > Is there somewhere it can be documented? A page with diagnostic commands? > Or even in the regular docs, with a warning of 'for developer use only' or > something? We do document appctl commands in ovs-vswitchd(8). It's OK to document them with a note that says that they are only meant for developers.
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); + 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; + int numa; + int vid; + int i; + + vid = netdev_dpdk_get_vid(dpdk_dev); + ds_put_format(ds, "vhost-user port: %s\n", netdev->name); + ds_put_format(ds, "\tMode: %s\n", client_mode ? "client" : "server"); + if (!rte_vhost_get_ifname(vid, socket_name, PATH_MAX)) { + 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); + } + + 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, + 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; + } + + 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); + 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); + 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]"
Add a command to dump vhost-user's information. Signed-off-by: Flavio Leitner <fbl@redhat.com> --- lib/netdev-dpdk.c | 124 ++++++++++++++++++++++++++++++++++++++++++++- vswitchd/ovs-vswitchd.8.in | 4 ++ 2 files changed, 126 insertions(+), 2 deletions(-)