[ovs-dev,v2,1/3] vswitchd: Document netdev-dpdk commands.

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
Acked-by: Antonio Fischetti <antonio.fischetti@intel.com>
---
 NEWS                        | 2 ++
 lib/automake.mk             | 1 +
 lib/netdev-dpdk-unixctl.man | 9 +++++++++
 manpages.mk                 | 2 ++
 vswitchd/ovs-vswitchd.8.in  | 1 +
 5 files changed, 15 insertions(+)
 create mode 100644 lib/netdev-dpdk-unixctl.man

>From: Ilya Maximets [mailto:i.maximets@samsung.com]
>Sent: Friday, November 10, 2017 7:12 AM
>To: ovs-dev@openvswitch.org
>Cc: Heetae Ahn <heetae82.ahn@samsung.com>; Fischetti, Antonio
><antonio.fischetti@intel.com>; Loftus, Ciara <ciara.loftus@intel.com>;
>Kavanagh, Mark B <mark.b.kavanagh@intel.com>; Stokes, Ian
><ian.stokes@intel.com>; Wojciechowicz, RobertX
><robertx.wojciechowicz@intel.com>; Flavio Leitner <fbl@redhat.com>; Ilya
>Maximets <i.maximets@samsung.com>
>Subject: [PATCH v2 1/3] vswitchd: Document netdev-dpdk commands.
>
>Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>Acked-by: Antonio Fischetti <antonio.fischetti@intel.com>

Hi Ilya,

Thanks for the patch.

At a high-level, it needs a rebase (to address conflicts in NEWS).

Apart from that, some minor comments inline.

Thanks,
Mark

>---
> NEWS                        | 2 ++
> lib/automake.mk             | 1 +
> lib/netdev-dpdk-unixctl.man | 9 +++++++++
> manpages.mk                 | 2 ++
> vswitchd/ovs-vswitchd.8.in  | 1 +
> 5 files changed, 15 insertions(+)
> create mode 100644 lib/netdev-dpdk-unixctl.man
>
>diff --git a/NEWS b/NEWS
>index a93237f..ccd409f 100644
>--- a/NEWS
>+++ b/NEWS
>@@ -12,6 +12,8 @@ Post-v2.8.0
>          IPv6 packets.
>    - Linux kernel 4.13
>      * Add support for compiling OVS with the latest Linux 4.13 kernel
>+   - DPDK:
>+     * All the netdev-dpdk appctl commands described in ovs-vswitchd man
>page.
>
> v2.8.0 - 31 Aug 2017
> --------------------
>diff --git a/lib/automake.mk b/lib/automake.mk
>index effe5b5..4b38a11 100644
>--- a/lib/automake.mk
>+++ b/lib/automake.mk
>@@ -465,6 +465,7 @@ MAN_FRAGMENTS += \
> 	lib/db-ctl-base.man \
> 	lib/dpctl.man \
> 	lib/memory-unixctl.man \
>+	lib/netdev-dpdk-unixctl.man \
> 	lib/ofp-version.man \
> 	lib/ovs.tmac \
> 	lib/service.man \
>diff --git a/lib/netdev-dpdk-unixctl.man b/lib/netdev-dpdk-unixctl.man
>new file mode 100644
>index 0000000..a4b7f60
>--- /dev/null
>+++ b/lib/netdev-dpdk-unixctl.man
>@@ -0,0 +1,9 @@
>+.SS "NETDEV-DPDK COMMANDS"
>+These commands manage DPDK related ports (\fItype=dpdk*\fR).
>+.IP "\fBnetdev-dpdk/set-admin-state\fR [\fIinterface\fR] \fIstate\fR"
>+Sets admin state for DPDK \fIinterface\fR (or all interfaces if none is
>given)
>+to \fIstate\fR.  \fIstate\fR can be "up" or "down".

A minor niggle, but we should probably remain consistent with how the command line is described by ovs-appctl list-commands:

	i.e   netdev-dpdk/set-admin-state [netdev] up|down

>+.IP "\fBnetdev-dpdk/detach\fR \fIpci-address\fR"
>+Detaches device with corresponding \fIpci-address\fR from DPDK.  This command
>+can be used to detach device if it wasn't detached automatically after port
>+deletion. Refer to the documentation for details and instructions.

Should probably point to specific documentation - there's a lot of it in OvS ;)

>diff --git a/manpages.mk b/manpages.mk
>index d610d88..c89bc45 100644
>--- a/manpages.mk
>+++ b/manpages.mk
>@@ -279,6 +279,7 @@ vswitchd/ovs-vswitchd.8: \
> 	lib/daemon.man \
> 	lib/dpctl.man \
> 	lib/memory-unixctl.man \
>+	lib/netdev-dpdk-unixctl.man \
> 	lib/service.man \
> 	lib/ssl-bootstrap.man \
> 	lib/ssl.man \
>@@ -296,6 +297,7 @@ lib/coverage-unixctl.man:
> lib/daemon.man:
> lib/dpctl.man:
> lib/memory-unixctl.man:
>+lib/netdev-dpdk-unixctl.man:
> lib/service.man:
> lib/ssl-bootstrap.man:
> lib/ssl.man:
>diff --git a/vswitchd/ovs-vswitchd.8.in b/vswitchd/ovs-vswitchd.8.in
>index c18baf6..76ccfcb 100644
>--- a/vswitchd/ovs-vswitchd.8.in
>+++ b/vswitchd/ovs-vswitchd.8.in
>@@ -283,6 +283,7 @@ port names, which this thread polls.
> .IP "\fBdpif-netdev/pmd-rxq-rebalance\fR [\fIdp\fR]"
> Reassigns rxqs to pmds in the datapath \fIdp\fR based on their current usage.
> .
>+.so lib/netdev-dpdk-unixctl.man
> .so ofproto/ofproto-dpif-unixctl.man
> .so ofproto/ofproto-unixctl.man
> .so lib/vlog-unixctl.man
>--
>2.7.4

On 08.12.2017 17:44, Kavanagh, Mark B wrote:
>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>> Sent: Friday, November 10, 2017 7:12 AM
>> To: ovs-dev@openvswitch.org
>> Cc: Heetae Ahn <heetae82.ahn@samsung.com>; Fischetti, Antonio
>> <antonio.fischetti@intel.com>; Loftus, Ciara <ciara.loftus@intel.com>;
>> Kavanagh, Mark B <mark.b.kavanagh@intel.com>; Stokes, Ian
>> <ian.stokes@intel.com>; Wojciechowicz, RobertX
>> <robertx.wojciechowicz@intel.com>; Flavio Leitner <fbl@redhat.com>; Ilya
>> Maximets <i.maximets@samsung.com>
>> Subject: [PATCH v2 1/3] vswitchd: Document netdev-dpdk commands.
>>
>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>> Acked-by: Antonio Fischetti <antonio.fischetti@intel.com>
> 
> Hi Ilya,
> 
> Thanks for the patch.

Thanks for review.

> 
> At a high-level, it needs a rebase (to address conflicts in NEWS).

This is kind of trivial. But I suspect that I'll need to do this once again
if DPDK 17.11 support will be merged.
From the other side, I already have rebased version locally, so, I could just
send it.

> 
> Apart from that, some minor comments inline.
> 
> Thanks,
> Mark
> 
>> ---
>> NEWS                        | 2 ++
>> lib/automake.mk             | 1 +
>> lib/netdev-dpdk-unixctl.man | 9 +++++++++
>> manpages.mk                 | 2 ++
>> vswitchd/ovs-vswitchd.8.in  | 1 +
>> 5 files changed, 15 insertions(+)
>> create mode 100644 lib/netdev-dpdk-unixctl.man
>>
>> diff --git a/NEWS b/NEWS
>> index a93237f..ccd409f 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -12,6 +12,8 @@ Post-v2.8.0
>>          IPv6 packets.
>>    - Linux kernel 4.13
>>      * Add support for compiling OVS with the latest Linux 4.13 kernel
>> +   - DPDK:
>> +     * All the netdev-dpdk appctl commands described in ovs-vswitchd man
>> page.
>>
>> v2.8.0 - 31 Aug 2017
>> --------------------
>> diff --git a/lib/automake.mk b/lib/automake.mk
>> index effe5b5..4b38a11 100644
>> --- a/lib/automake.mk
>> +++ b/lib/automake.mk
>> @@ -465,6 +465,7 @@ MAN_FRAGMENTS += \
>> 	lib/db-ctl-base.man \
>> 	lib/dpctl.man \
>> 	lib/memory-unixctl.man \
>> +	lib/netdev-dpdk-unixctl.man \
>> 	lib/ofp-version.man \
>> 	lib/ovs.tmac \
>> 	lib/service.man \
>> diff --git a/lib/netdev-dpdk-unixctl.man b/lib/netdev-dpdk-unixctl.man
>> new file mode 100644
>> index 0000000..a4b7f60
>> --- /dev/null
>> +++ b/lib/netdev-dpdk-unixctl.man
>> @@ -0,0 +1,9 @@
>> +.SS "NETDEV-DPDK COMMANDS"
>> +These commands manage DPDK related ports (\fItype=dpdk*\fR).
>> +.IP "\fBnetdev-dpdk/set-admin-state\fR [\fIinterface\fR] \fIstate\fR"
>> +Sets admin state for DPDK \fIinterface\fR (or all interfaces if none is
>> given)
>> +to \fIstate\fR.  \fIstate\fR can be "up" or "down".
> 
> A minor niggle, but we should probably remain consistent with how the command line is described by ovs-appctl list-commands:
> 
> 	i.e   netdev-dpdk/set-admin-state [netdev] up|down


I'm not sure about that. There is no ovs-appctl commands in man page
described that way, but there are 2 commands: 'bfd/set-forwarding' and
'cfm/set-fault' that described like "[interface] status" in man page but
"[interface] normal|false|true" in ovs-appctl list-commands.

So, I prefer to keep current version to keep man pages consistent.
What do you think?

> 
>> +.IP "\fBnetdev-dpdk/detach\fR \fIpci-address\fR"
>> +Detaches device with corresponding \fIpci-address\fR from DPDK.  This command
>> +can be used to detach device if it wasn't detached automatically after port
>> +deletion. Refer to the documentation for details and instructions.
> 
> Should probably point to specific documentation - there's a lot of it in OvS ;)

Yes there are a lot of documentation, but it's hard to say how it'll be shipped
to the end user. It could be pdf docs, or html, or something else. I'm not sure
how to point the specific document. I tried to avoid pointing to something not
in the man pages. IMHO, it's the most safe approach to keep them consistent.
Also, there are few other places in same ovs-vswitchd man page that uses same
wording.

> 
>> diff --git a/manpages.mk b/manpages.mk
>> index d610d88..c89bc45 100644
>> --- a/manpages.mk
>> +++ b/manpages.mk
>> @@ -279,6 +279,7 @@ vswitchd/ovs-vswitchd.8: \
>> 	lib/daemon.man \
>> 	lib/dpctl.man \
>> 	lib/memory-unixctl.man \
>> +	lib/netdev-dpdk-unixctl.man \
>> 	lib/service.man \
>> 	lib/ssl-bootstrap.man \
>> 	lib/ssl.man \
>> @@ -296,6 +297,7 @@ lib/coverage-unixctl.man:
>> lib/daemon.man:
>> lib/dpctl.man:
>> lib/memory-unixctl.man:
>> +lib/netdev-dpdk-unixctl.man:
>> lib/service.man:
>> lib/ssl-bootstrap.man:
>> lib/ssl.man:
>> diff --git a/vswitchd/ovs-vswitchd.8.in b/vswitchd/ovs-vswitchd.8.in
>> index c18baf6..76ccfcb 100644
>> --- a/vswitchd/ovs-vswitchd.8.in
>> +++ b/vswitchd/ovs-vswitchd.8.in
>> @@ -283,6 +283,7 @@ port names, which this thread polls.
>> .IP "\fBdpif-netdev/pmd-rxq-rebalance\fR [\fIdp\fR]"
>> Reassigns rxqs to pmds in the datapath \fIdp\fR based on their current usage.
>> .
>> +.so lib/netdev-dpdk-unixctl.man
>> .so ofproto/ofproto-dpif-unixctl.man
>> .so ofproto/ofproto-unixctl.man
>> .so lib/vlog-unixctl.man
>> --
>> 2.7.4

>From: Ilya Maximets [mailto:i.maximets@samsung.com]
>Sent: Friday, December 8, 2017 3:26 PM
>To: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; ovs-dev@openvswitch.org
>Cc: Heetae Ahn <heetae82.ahn@samsung.com>; Fischetti, Antonio
><antonio.fischetti@intel.com>; Loftus, Ciara <ciara.loftus@intel.com>; Stokes,
>Ian <ian.stokes@intel.com>; Wojciechowicz, RobertX
><robertx.wojciechowicz@intel.com>; Flavio Leitner <fbl@redhat.com>
>Subject: Re: [PATCH v2 1/3] vswitchd: Document netdev-dpdk commands.
>
>On 08.12.2017 17:44, Kavanagh, Mark B wrote:
>>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>>> Sent: Friday, November 10, 2017 7:12 AM
>>> To: ovs-dev@openvswitch.org
>>> Cc: Heetae Ahn <heetae82.ahn@samsung.com>; Fischetti, Antonio
>>> <antonio.fischetti@intel.com>; Loftus, Ciara <ciara.loftus@intel.com>;
>>> Kavanagh, Mark B <mark.b.kavanagh@intel.com>; Stokes, Ian
>>> <ian.stokes@intel.com>; Wojciechowicz, RobertX
>>> <robertx.wojciechowicz@intel.com>; Flavio Leitner <fbl@redhat.com>; Ilya
>>> Maximets <i.maximets@samsung.com>
>>> Subject: [PATCH v2 1/3] vswitchd: Document netdev-dpdk commands.
>>>
>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>> Acked-by: Antonio Fischetti <antonio.fischetti@intel.com>
>>
>> Hi Ilya,
>>
>> Thanks for the patch.
>
>Thanks for review.
>
>>
>> At a high-level, it needs a rebase (to address conflicts in NEWS).
>
>This is kind of trivial. But I suspect that I'll need to do this once again
>if DPDK 17.11 support will be merged.
>From the other side, I already have rebased version locally, so, I could just
>send it.
>
>>
>> Apart from that, some minor comments inline.
>>
>> Thanks,
>> Mark
>>
>>> ---
>>> NEWS                        | 2 ++
>>> lib/automake.mk             | 1 +
>>> lib/netdev-dpdk-unixctl.man | 9 +++++++++
>>> manpages.mk                 | 2 ++
>>> vswitchd/ovs-vswitchd.8.in  | 1 +
>>> 5 files changed, 15 insertions(+)
>>> create mode 100644 lib/netdev-dpdk-unixctl.man
>>>
>>> diff --git a/NEWS b/NEWS
>>> index a93237f..ccd409f 100644
>>> --- a/NEWS
>>> +++ b/NEWS
>>> @@ -12,6 +12,8 @@ Post-v2.8.0
>>>          IPv6 packets.
>>>    - Linux kernel 4.13
>>>      * Add support for compiling OVS with the latest Linux 4.13 kernel
>>> +   - DPDK:
>>> +     * All the netdev-dpdk appctl commands described in ovs-vswitchd man
>>> page.
>>>
>>> v2.8.0 - 31 Aug 2017
>>> --------------------
>>> diff --git a/lib/automake.mk b/lib/automake.mk
>>> index effe5b5..4b38a11 100644
>>> --- a/lib/automake.mk
>>> +++ b/lib/automake.mk
>>> @@ -465,6 +465,7 @@ MAN_FRAGMENTS += \
>>> 	lib/db-ctl-base.man \
>>> 	lib/dpctl.man \
>>> 	lib/memory-unixctl.man \
>>> +	lib/netdev-dpdk-unixctl.man \
>>> 	lib/ofp-version.man \
>>> 	lib/ovs.tmac \
>>> 	lib/service.man \
>>> diff --git a/lib/netdev-dpdk-unixctl.man b/lib/netdev-dpdk-unixctl.man
>>> new file mode 100644
>>> index 0000000..a4b7f60
>>> --- /dev/null
>>> +++ b/lib/netdev-dpdk-unixctl.man
>>> @@ -0,0 +1,9 @@
>>> +.SS "NETDEV-DPDK COMMANDS"
>>> +These commands manage DPDK related ports (\fItype=dpdk*\fR).
>>> +.IP "\fBnetdev-dpdk/set-admin-state\fR [\fIinterface\fR] \fIstate\fR"
>>> +Sets admin state for DPDK \fIinterface\fR (or all interfaces if none is
>>> given)
>>> +to \fIstate\fR.  \fIstate\fR can be "up" or "down".
>>
>> A minor niggle, but we should probably remain consistent with how the
>command line is described by ovs-appctl list-commands:
>>
>> 	i.e   netdev-dpdk/set-admin-state [netdev] up|down
>
>
>I'm not sure about that. There is no ovs-appctl commands in man page
>described that way, but there are 2 commands: 'bfd/set-forwarding' and
>'cfm/set-fault' that described like "[interface] status" in man page but
>"[interface] normal|false|true" in ovs-appctl list-commands.
>
>So, I prefer to keep current version to keep man pages consistent.
>What do you think?

In all honesty, I'd probably change all of them to be consistent with the list-commands output, but that's for another day.

I'm happy to stick with the existing 'convention' for now.

>
>>
>>> +.IP "\fBnetdev-dpdk/detach\fR \fIpci-address\fR"
>>> +Detaches device with corresponding \fIpci-address\fR from DPDK.  This
>command
>>> +can be used to detach device if it wasn't detached automatically after
>port
>>> +deletion. Refer to the documentation for details and instructions.
>>
>> Should probably point to specific documentation - there's a lot of it in OvS
>;)
>
>Yes there are a lot of documentation, but it's hard to say how it'll be
>shipped
>to the end user. It could be pdf docs, or html, or something else. I'm not
>sure

Sure - the format isn't important, and there's no need to be super-specific.
A general landmark should suffice - e.g. "the 'Port Hotplug' section of the DPDK-specific documentation".

>how to point the specific document. I tried to avoid pointing to something not
>in the man pages. IMHO, it's the most safe approach to keep them consistent.
>Also, there are few other places in same ovs-vswitchd man page that uses same
>wording.

Sure, but that doesn't necessarily validate that approach IMHO.

>
>>
>>> diff --git a/manpages.mk b/manpages.mk
>>> index d610d88..c89bc45 100644
>>> --- a/manpages.mk
>>> +++ b/manpages.mk
>>> @@ -279,6 +279,7 @@ vswitchd/ovs-vswitchd.8: \
>>> 	lib/daemon.man \
>>> 	lib/dpctl.man \
>>> 	lib/memory-unixctl.man \
>>> +	lib/netdev-dpdk-unixctl.man \
>>> 	lib/service.man \
>>> 	lib/ssl-bootstrap.man \
>>> 	lib/ssl.man \
>>> @@ -296,6 +297,7 @@ lib/coverage-unixctl.man:
>>> lib/daemon.man:
>>> lib/dpctl.man:
>>> lib/memory-unixctl.man:
>>> +lib/netdev-dpdk-unixctl.man:
>>> lib/service.man:
>>> lib/ssl-bootstrap.man:
>>> lib/ssl.man:
>>> diff --git a/vswitchd/ovs-vswitchd.8.in b/vswitchd/ovs-vswitchd.8.in
>>> index c18baf6..76ccfcb 100644
>>> --- a/vswitchd/ovs-vswitchd.8.in
>>> +++ b/vswitchd/ovs-vswitchd.8.in
>>> @@ -283,6 +283,7 @@ port names, which this thread polls.
>>> .IP "\fBdpif-netdev/pmd-rxq-rebalance\fR [\fIdp\fR]"
>>> Reassigns rxqs to pmds in the datapath \fIdp\fR based on their current
>usage.
>>> .
>>> +.so lib/netdev-dpdk-unixctl.man
>>> .so ofproto/ofproto-dpif-unixctl.man
>>> .so ofproto/ofproto-unixctl.man
>>> .so lib/vlog-unixctl.man
>>> --
>>> 2.7.4