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

[Kavanagh, Mark B]

>From: Ilya Maximets [mailto:i.maxim...@samsung.com]
>Sent: Friday, December 8, 2017 3:26 PM
>To: Kavanagh, Mark B ; ovs-dev@openvswitch.org
>Cc: Heetae Ahn ; Fischetti, Antonio
>; Loftus, Ciara ; Stokes,
>Ian ; Wojciechowicz, RobertX
>; Flavio Leitner 
>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.maxim...@samsung.com]
>>> Sent: Friday, November 10, 2017 7:12 AM
>>> To: ovs-dev@openvswitch.org
>>> Cc: Heetae Ahn ; Fischetti, Antonio
>>> ; Loftus, Ciara ;
>>> Kavanagh, Mark B ; Stokes, Ian
>>> ; Wojciechowicz, RobertX
>>> ; Flavio Leitner ; Ilya
>>> Maximets 
>>> Subject: [PATCH v2 1/3] vswitchd: Document netdev-dpdk commands.
>>>
>>> Signed-off-by: Ilya Maximets 
>>> Acked-by: Antonio Fischetti 
>>
>> 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 000..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 

Re: [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.maxim...@samsung.com]
>> Sent: Friday, November 10, 2017 7:12 AM
>> To: ovs-dev@openvswitch.org
>> Cc: Heetae Ahn ; Fischetti, Antonio
>> ; Loftus, Ciara ;
>> Kavanagh, Mark B ; Stokes, Ian
>> ; Wojciechowicz, RobertX
>> ; Flavio Leitner ; Ilya
>> Maximets 
>> Subject: [PATCH v2 1/3] vswitchd: Document netdev-dpdk commands.
>>
>> Signed-off-by: Ilya Maximets 
>> Acked-by: Antonio Fischetti 
> 
> 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 000..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 

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

[Kavanagh, Mark B]

>From: Ilya Maximets [mailto:i.maxim...@samsung.com]
>Sent: Friday, November 10, 2017 7:12 AM
>To: ovs-dev@openvswitch.org
>Cc: Heetae Ahn ; Fischetti, Antonio
>; Loftus, Ciara ;
>Kavanagh, Mark B ; Stokes, Ian
>; Wojciechowicz, RobertX
>; Flavio Leitner ; Ilya
>Maximets 
>Subject: [PATCH v2 1/3] vswitchd: Document netdev-dpdk commands.
>
>Signed-off-by: Ilya Maximets 
>Acked-by: Antonio Fischetti 

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 000..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

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Ilya Maximets]

Signed-off-by: Ilya Maximets 
Acked-by: Antonio Fischetti 
---
 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 000..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".
+.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.
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

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev