[ovs-dev,v3] ovs-vsctl: show DPDK version

Message ID 20171225143102.24335-1-mcroce@redhat.com
State New
Headers show
Series
  • [ovs-dev,v3] ovs-vsctl: show DPDK version
Related show

Commit Message

Matteo Croce Dec. 25, 2017, 2:31 p.m.
Show DPDK version if Open vSwitch is compiled with DPDK support.
Add a `dpdk_version` column in the datamodel, change ovs-ctl and ovs-dev.py
to populate the field with the right value.

Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
 utilities/ovs-ctl.in       | 9 ++++++++-
 utilities/ovs-dev.py       | 7 +++++++
 vswitchd/ovs-vswitchd.c    | 7 +++++++
 vswitchd/vswitch.ovsschema | 7 +++++--
 vswitchd/vswitch.xml       | 4 ++++
 5 files changed, 31 insertions(+), 3 deletions(-)

Comments

Ben Pfaff Dec. 26, 2017, 5:38 p.m. | #1
On Mon, Dec 25, 2017 at 03:31:02PM +0100, Matteo Croce wrote:
> Show DPDK version if Open vSwitch is compiled with DPDK support.
> Add a `dpdk_version` column in the datamodel, change ovs-ctl and ovs-dev.py
> to populate the field with the right value.
> 
> Signed-off-by: Matteo Croce <mcroce@redhat.com>

OK, this is better, but why does ovs-vswitchd need a shell helper for
this?  It should just set the key itself.
Matteo Croce Dec. 26, 2017, 6:48 p.m. | #2
On Tue, Dec 26, 2017 at 6:38 PM, Ben Pfaff <blp@ovn.org> wrote:
> On Mon, Dec 25, 2017 at 03:31:02PM +0100, Matteo Croce wrote:
>> Show DPDK version if Open vSwitch is compiled with DPDK support.
>> Add a `dpdk_version` column in the datamodel, change ovs-ctl and ovs-dev.py
>> to populate the field with the right value.
>>
>> Signed-off-by: Matteo Croce <mcroce@redhat.com>
>
> OK, this is better, but why does ovs-vswitchd need a shell helper for
> this?  It should just set the key itself.

On ovs-vswitchd startup?
I think I set dpdk_version the same way ovs_version was set.
I can do it in ovs-vswitchd startup but in that case ovs_version
should be set in ovs-vswitchd too.

Cheers,
Aaron Conole Jan. 2, 2018, 5:08 p.m. | #3
Hi Matteo,

Matteo Croce <mcroce@redhat.com> writes:

> Show DPDK version if Open vSwitch is compiled with DPDK support.
> Add a `dpdk_version` column in the datamodel, change ovs-ctl and ovs-dev.py
> to populate the field with the right value.
>
> Signed-off-by: Matteo Croce <mcroce@redhat.com>
> ---

Sorry I missed some of the development for this - I think it's better to
have the dpdk version retrieved via an appctl command.  I can't think of
a need for this information to be persistent - if it's for historical
information when debugging, we can put it in with the logs when dpdk is
started.  The user cannot influence DPDK version dynamically - it is not
just read-only, but it's read-only from a compile time decision.  I
might be misunderstanding the point of putting this in the DB, though?
Matteo Croce Jan. 2, 2018, 5:18 p.m. | #4
On Tue, Jan 2, 2018 at 6:08 PM, Aaron Conole <aconole@redhat.com> wrote:
> Hi Matteo,
>
> Matteo Croce <mcroce@redhat.com> writes:
>
>> Show DPDK version if Open vSwitch is compiled with DPDK support.
>> Add a `dpdk_version` column in the datamodel, change ovs-ctl and ovs-dev.py
>> to populate the field with the right value.
>>
>> Signed-off-by: Matteo Croce <mcroce@redhat.com>
>> ---
>
> Sorry I missed some of the development for this - I think it's better to
> have the dpdk version retrieved via an appctl command.  I can't think of
> a need for this information to be persistent - if it's for historical
> information when debugging, we can put it in with the logs when dpdk is
> started.  The user cannot influence DPDK version dynamically - it is not
> just read-only, but it's read-only from a compile time decision.  I
> might be misunderstanding the point of putting this in the DB, though?

Hi Aaron,

I was threating dpdk_version the same way as ovs_version.
ovs_version is saved into the DB by ovs-ctl and it isn't read-only either.
What about putting it only in `ovs-vswitchd --version` output and not
into the DB?
Ben Pfaff Jan. 2, 2018, 6:41 p.m. | #5
On Tue, Jan 02, 2018 at 06:18:03PM +0100, Matteo Croce wrote:
> On Tue, Jan 2, 2018 at 6:08 PM, Aaron Conole <aconole@redhat.com> wrote:
> > Hi Matteo,
> >
> > Matteo Croce <mcroce@redhat.com> writes:
> >
> >> Show DPDK version if Open vSwitch is compiled with DPDK support.
> >> Add a `dpdk_version` column in the datamodel, change ovs-ctl and ovs-dev.py
> >> to populate the field with the right value.
> >>
> >> Signed-off-by: Matteo Croce <mcroce@redhat.com>
> >> ---
> >
> > Sorry I missed some of the development for this - I think it's better to
> > have the dpdk version retrieved via an appctl command.  I can't think of
> > a need for this information to be persistent - if it's for historical
> > information when debugging, we can put it in with the logs when dpdk is
> > started.  The user cannot influence DPDK version dynamically - it is not
> > just read-only, but it's read-only from a compile time decision.  I
> > might be misunderstanding the point of putting this in the DB, though?
> 
> Hi Aaron,
> 
> I was threating dpdk_version the same way as ovs_version.
> ovs_version is saved into the DB by ovs-ctl and it isn't read-only either.
> What about putting it only in `ovs-vswitchd --version` output and not
> into the DB?

One reason that ovs_version is in the database is because a controller
might need to know and the controller can't necessarily run
"ovs-vswitchd --version" since it might be on a different host.

I don't know whether a controller needs to know the DPDK version.  I
assumed that it did and that that was the motivation for the commit.
Aaron Conole Jan. 2, 2018, 6:50 p.m. | #6
Matteo Croce <mcroce@redhat.com> writes:

> On Tue, Jan 2, 2018 at 6:08 PM, Aaron Conole <aconole@redhat.com> wrote:
>> Hi Matteo,
>>
>> Matteo Croce <mcroce@redhat.com> writes:
>>
>>> Show DPDK version if Open vSwitch is compiled with DPDK support.
>>> Add a `dpdk_version` column in the datamodel, change ovs-ctl and ovs-dev.py
>>> to populate the field with the right value.
>>>
>>> Signed-off-by: Matteo Croce <mcroce@redhat.com>
>>> ---
>>
>> Sorry I missed some of the development for this - I think it's better to
>> have the dpdk version retrieved via an appctl command.  I can't think of
>> a need for this information to be persistent - if it's for historical
>> information when debugging, we can put it in with the logs when dpdk is
>> started.  The user cannot influence DPDK version dynamically - it is not
>> just read-only, but it's read-only from a compile time decision.  I
>> might be misunderstanding the point of putting this in the DB, though?
>
> Hi Aaron,
>
> I was threating dpdk_version the same way as ovs_version.

Ahh, okay.  I had forgotten that ovs_version (and other information) was
in the db as well.  Sorry for that - next time I'll have coffee before
reviewing. :)

> ovs_version is saved into the DB by ovs-ctl and it isn't read-only either.
> What about putting it only in `ovs-vswitchd --version` output and not
> into the DB?

What about using the datapath_version field?  That field already gets
populated, and I think it may be okay to modify the return value of
dpif_netdev_get_datapath_version function when dpdk is enabled.  Just a
thought.
Vishal Deep Ajmera Jan. 4, 2018, 10:50 a.m. | #7
Hi Matteo,

Can we also have DPDK version in the logs (VLOG_INFO) when DPDK is initialized ? May be in dpdk_init() function. This will help in identifying the correct DPDK version from the log files while debugging issues.

Warm Regards,
Vishal Ajmera

-----Original Message-----
From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Aaron Conole
Sent: Wednesday, January 03, 2018 12:20 AM
To: Matteo Croce <mcroce@redhat.com>
Cc: dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH v3] ovs-vsctl: show DPDK version

Matteo Croce <mcroce@redhat.com> writes:

> On Tue, Jan 2, 2018 at 6:08 PM, Aaron Conole <aconole@redhat.com> wrote:
>> Hi Matteo,
>>
>> Matteo Croce <mcroce@redhat.com> writes:
>>
>>> Show DPDK version if Open vSwitch is compiled with DPDK support.
>>> Add a `dpdk_version` column in the datamodel, change ovs-ctl and 
>>> ovs-dev.py to populate the field with the right value.
>>>
>>> Signed-off-by: Matteo Croce <mcroce@redhat.com>
>>> ---
>>
>> Sorry I missed some of the development for this - I think it's better 
>> to have the dpdk version retrieved via an appctl command.  I can't 
>> think of a need for this information to be persistent - if it's for 
>> historical information when debugging, we can put it in with the logs 
>> when dpdk is started.  The user cannot influence DPDK version 
>> dynamically - it is not just read-only, but it's read-only from a 
>> compile time decision.  I might be misunderstanding the point of putting this in the DB, though?
>
> Hi Aaron,
>
> I was threating dpdk_version the same way as ovs_version.

Ahh, okay.  I had forgotten that ovs_version (and other information) was in the db as well.  Sorry for that - next time I'll have coffee before reviewing. :)

> ovs_version is saved into the DB by ovs-ctl and it isn't read-only either.
> What about putting it only in `ovs-vswitchd --version` output and not 
> into the DB?

What about using the datapath_version field?  That field already gets populated, and I think it may be okay to modify the return value of dpif_netdev_get_datapath_version function when dpdk is enabled.  Just a thought.
Matteo Croce Jan. 4, 2018, 10:54 a.m. | #8
On Thu, Jan 4, 2018 at 11:50 AM, Vishal Deep Ajmera
<vishal.deep.ajmera@ericsson.com> wrote:
> Hi Matteo,
>
> Can we also have DPDK version in the logs (VLOG_INFO) when DPDK is initialized ? May be in dpdk_init() function. This will help in identifying the correct DPDK version from the log files while debugging issues.
>
> Warm Regards,
> Vishal Ajmera
>

Sure, makes sense.

Thanks,

Patch

diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
index f1b01d1d3..1e334de2b 100755
--- a/utilities/ovs-ctl.in
+++ b/utilities/ovs-ctl.in
@@ -76,9 +76,16 @@  set_hostname () {
 set_system_ids () {
     set ovs_vsctl set Open_vSwitch .
 
-    OVS_VERSION=`ovs-vswitchd --version | sed 's/.*) //;1q'`
+    OVS_VERSION=`ovs-vswitchd --version |awk '/Open vSwitch/{print $NF}'`
     set "$@" ovs-version="$OVS_VERSION"
 
+    DPDK_VERSION=`ovs-vswitchd --version |awk '/^DPDK/{print$2}'`
+    if [ -n "$DPDK_VERSION" ]; then
+        set "$@" dpdk-version="$DPDK_VERSION"
+    else
+        ovs_vsctl clear Open_vSwitch . dpdk_version
+    fi
+
     case $SYSTEM_ID in
         random)
             id_file=$etcdir/system-id.conf
diff --git a/utilities/ovs-dev.py b/utilities/ovs-dev.py
index 9ce0f04c7..70170e21d 100755
--- a/utilities/ovs-dev.py
+++ b/utilities/ovs-dev.py
@@ -284,9 +284,16 @@  def run():
             "other_config:dpdk-init=true" % root_uuid)
         _sh("ovs-vsctl --no-wait set Open_vSwitch %s other_config:"
             "dpdk-extra=\"%s\"" % (root_uuid, ' '.join(options.dpdk)))
+        dpdk_version = _sh("ovs-vswitchd --version", capture=True)
+        dpdk_version = [s for s in dpdk_version if "DPDK" in s]
+        if dpdk_version:
+            dpdk_version = dpdk_version[0].decode().strip().split()[1]
+            _sh("ovs-vsctl --no-wait set Open_vSwitch %s dpdk_version=%s"
+                % (root_uuid, dpdk_version))
     else:
         _sh("ovs-vsctl --no-wait set Open_vSwitch %s "
             "other_config:dpdk-init=false" % root_uuid)
+        _sh("ovs-vsctl --no-wait clear Open_vSwitch %s dpdk_version" % root_uuid)
 
     if options.gdb:
         cmd = ["gdb", "--args"] + cmd
diff --git a/vswitchd/ovs-vswitchd.c b/vswitchd/ovs-vswitchd.c
index d5e07c037..e033a7d1f 100644
--- a/vswitchd/ovs-vswitchd.c
+++ b/vswitchd/ovs-vswitchd.c
@@ -25,6 +25,10 @@ 
 #include <sys/mman.h>
 #endif
 
+#ifdef DPDK_NETDEV
+#include <rte_version.h>
+#endif
+
 #include "bridge.h"
 #include "command-line.h"
 #include "compiler.h"
@@ -187,6 +191,9 @@  parse_options(int argc, char *argv[], char **unixctl_pathp)
 
         case 'V':
             ovs_print_version(0, 0);
+#ifdef DPDK_NETDEV
+            puts(rte_version());
+#endif
             exit(EXIT_SUCCESS);
 
         case OPT_MLOCKALL:
diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
index 90e50b626..7015a2687 100644
--- a/vswitchd/vswitch.ovsschema
+++ b/vswitchd/vswitch.ovsschema
@@ -1,6 +1,6 @@ 
 {"name": "Open_vSwitch",
- "version": "7.15.1",
- "cksum": "3682332033 23608",
+ "version": "7.16.0",
+ "cksum": "654116098 23718",
  "tables": {
    "Open_vSwitch": {
      "columns": {
@@ -36,6 +36,9 @@ 
        "db_version": {
          "type": {"key": {"type": "string"},
                   "min": 0, "max": 1}},
+       "dpdk_version": {
+         "type": {"key": {"type": "string"},
+                  "min": 0, "max": 1}},
        "system_type": {
          "type": {"key": {"type": "string"},
                   "min": 0, "max": 1}},
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 37d04b7cf..74890f72b 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -611,6 +611,10 @@ 
         </p>
       </column>
 
+      <column name="dpdk_version">
+        DPDK version number, e.g. <code>17.11</code>.
+      </column>
+
       <column name="system_type">
         <p>
           An identifier for the type of system on top of which Open vSwitch