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

[Ilya Maximets]

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