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 <f...@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]"
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
