Re: [ovs-dev] [PATCH] dpdk: announce deprecation of vhost-user server ports

[Aaron Conole]

Hi Sean, and Mark,

"Mooney, Sean K" <sean.k.moo...@intel.com> writes:

>> -Original Message-
>> From: Aaron Conole [mailto:acon...@redhat.com]
>> Sent: Thursday, June 8, 2017 8:12 PM
>> To: Kevin Traynor <ktray...@redhat.com>
>> Cc: d...@openvswitch.org; Darrell Ball <db...@vmware.com>; Loftus, Ciara
>> <ciara.lof...@intel.com>; Mooney, Sean K <sean.k.moo...@intel.com>
>> Subject: Re: [PATCH] dpdk: announce deprecation of vhost-user server
>> ports
>> 
>> Hi Kevin,
>> 
>> Kevin Traynor <ktray...@redhat.com> writes:
>> 
>> > On 06/07/2017 11:46 PM, Aaron Conole wrote:
>> >> Since vhost-user server mode ports are the preferred mechanism for
>> >> interconnecting Open vSwitch with VMs when using DPDK, and since
>> >> there are currently no known use cases for vhost-user server mode
>> >> ports apart from version incompatibilities with QEMU, announce that
>> >> server mode ports are considered deprecated and will be removed in a
>> future release.
>
> [Mooney, Sean K] not to be pedantic but you contradicted your self her. First 
> sentence
> You say vhost-user server mode ports are preferred then you say lets remove 
> them.
> I would suggest you use the interface names instead and say
> dpdkvhostuser when referring to what will be removed
> Server mode port is ambigious since its not clear if you are referring
> to qemu or dpdk when you say server mode.

The mistake was pointed out - unfortunately it was already applied.

Sorry for the confusion.

>> >>
>> >> Cc: Ciara Loftus <ciara.lof...@intel.com>
>> >> Cc: Kevin Traynor <ktray...@redhat.com>
>> >> Suggested-by: Darrell Ball <db...@vmware.com>
>> >> Signed-off-by: Aaron Conole <acon...@redhat.com>
>> >> ---
>> >>  Documentation/topics/dpdk/vhost-user.rst | 24 -
>> ---
>> >>  NEWS |  2 ++
>> >>  lib/netdev-dpdk.c|  2 ++
>> >>  3 files changed, 20 insertions(+), 8 deletions(-)
>> >>
>> >> diff --git a/Documentation/topics/dpdk/vhost-user.rst
>> >> b/Documentation/topics/dpdk/vhost-user.rst
>> >> index a1c19fd..9d36cf2 100644
>> >> --- a/Documentation/topics/dpdk/vhost-user.rst
>> >> +++ b/Documentation/topics/dpdk/vhost-user.rst
>> >> @@ -32,13 +32,19 @@ documentation`_ on same.
>> >>  Quick Example
>> >>  -
>> >>
>> >> -This example demonstrates how to add two ``dpdkvhostuser`` ports to
>> >> an existing -bridge called ``br0``::
>> >> +This example demonstrates how to add two ``dpdkvhostuserclient``
>> >> +ports to an existing bridge called ``br0``::
>> >>
>> >> -$ ovs-vsctl add-port br0 dpdkvhostuser0 \
>> >> --- set Interface dpdkvhostuser0 type=dpdkvhostuser
>> >> -$ ovs-vsctl add-port br0 dpdkvhostuser1 \
>> >> --- set Interface dpdkvhostuser1 type=dpdkvhostuser
>> >> +$ ovs-vsctl add-port br0 dpdkvhostclient0 \
>> >> +-- set Interface dpdkvhostclient0 type=dpdkvhostuserclient
>> \
>> >> +   options:vhost-server-path=/tmp/dpdkvhostclient0
>> >> +$ ovs-vsctl add-port br0 dpdkvhostclient1 \
>> >> +-- set Interface dpdkvhostclient1 type=dpdkvhostuserclient
>> \
>> >> +   options:vhost-server-path=/tmp/dpdkvhostclient1
>> >> +
>> >> +For the above examples to work, an appropriate server socket must
>> be
>> >> +created at the paths specified (``/tmp/dpdkvhostclient0`` and
>> >> +``/tmp/dpdkvhostclient0``).
>> >
>> > You could mention QEMU here. So the reader knows where to look.
>> > "These can be created by QEMU. See below for details."?
>> 
>> Good idea.  I'll add it.
>> 
>> Thanks for the review!
>> 
>> >>  vhost-user vs. vhost-user-client
>> >>  
>> >> @@ -59,7 +65,8 @@ means if OVS dies, all VMs **must** be restarted.
>> >> On the other hand, for  vhost-user-client ports, OVS acts as the
>> >> client and QEMU the server. This means  OVS can die and be restarted
>> >> without issue, and it is also possible to restart  an instance
>> >> itself. For this reason, vhost-user-client ports are the preferred -
>> type for most use cases.
>> >> +type for most use cases. 

Re: [ovs-dev] [PATCH V2] netdev-dpdk: use rte_eth_dev_set_mtu

[Aaron Conole]

Mark Kavanagh <mark.b.kavan...@intel.com> writes:

> DPDK provides an API to set the MTU of compatible physical devices -
> rte_eth_dev_set_mtu(). Prior to DPDK v16.07 however, this API was not
> implemented in some DPDK PMDs (i40e, specifically). To allow the use
> of jumbo frames with affected NICs in OvS-DPDK, MTU configuration was
> achieved by setting the jumbo frame flag, and corresponding maximum
> permitted Rx frame size, in an rte_eth_conf structure for the NIC
> port, and subsequently invoking rte_eth_dev_configure() with that
> configuration.
>
> However, that method does not set the MTU field of the underlying DPDK
> structure (rte_eth_dev) for the corresponding physical device;
> consequently, rte_eth_dev_get_mtu() reports the incorrect MTU for an
> OvS-DPDK phy device with non-standard MTU.
>
> Resolve this issue by invoking rte_eth_dev_set_mtu() when setting up
> or modifying the MTU of a DPDK phy port.
>
> Fixes: 0072e93 ("netdev-dpdk: add support for jumbo frames")
> Reported-by: Aaron Conole <acon...@redhat.com>
> Reported-by: Vipin Varghese <vipin.vargh...@intel.com>
> Signed-off-by: Mark Kavanagh <mark.b.kavan...@intel.com>
> ---

I won't have a chance to test this near-term, but it looks correct to
me.  Thanks for this work, Mark!

Reviewed-by: Aaron Conole <acon...@redhat.com>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] rte_eal_init() error when using ovs-dpdk with secondary application.

[Aaron Conole]

Junguk Cho <jman...@gmail.com> writes:

> Hi, Aaron.
>
> Thank you for reply.
>
> You will need to use populate this option into other_config:dpdk-extra
> in the ovsdb.
>
>   ovs-vsctl --no-wait set Open_vSwitch \
>   . other_config:dpdk-extra="--base-virtaddr=..."
>
> -> I tried to use "struct rte_memseg *m = rte_eal_get_physmem_layout()" 
> option based on
> (http://dpdk.org/doc/api/structrte__memseg.html).
> Always it returns different number. 
> How do I get virtual address value to use it as an input of "base-viraddr"?
>
> I don't know the base virtual address value you should use, however.
> -> Do you mean it will not help? 

I mean I don't know what value to use.

> Thanks,
> Junguk
>
> On Thu, Jun 15, 2017 at 9:28 AM, Aaron Conole <acon...@redhat.com> wrote:
>
>  Junguk Cho <jman...@gmail.com> writes:
>
>  > Hi,
>  >
>  > I use ovs-dpdk (ovs-2.7, dpdk-16.11.1) with one application which talks to
>  > ovs by using ring device and "--proc-type=secondary" (secondary processes).
>  > It generally works well, but sometimes it shows this error.
>  >
>  > It seems it could not find correct memory mapping in the application.
>  >
>  > 2017-06-13 11:21:40.988 STDERR Thread-2 [INFO] EAL: Probing VFIO support...
>  > 2017-06-13 11:21:40.988 STDERR Thread-2 [INFO] EAL: WARNING: Address Space
>  > Layout Randomization (ASLR) is enabled in the kernel.
>  > 2017-06-13 11:21:40.988 STDERR Thread-2 [INFO] EAL:This may cause
>  > issues with mapping memory into secondary processes
>  > 2017-06-13 11:21:40.989 STDERR Thread-2 [INFO] EAL: Could not mmap
>  > 17146314752 <(714)%20631-4752> bytes in /dev/zero at [0x7f662c80], got
>  > [0x7f650600] - please use '--base-virtaddr' option
>  > 2017-06-13 11:21:40.989 STDERR Thread-2 [INFO] EAL: It is recommended to
>  > disable ASLR in the kernel and retry running both primary and secondary
>  > processes
>  > 2017-06-13 11:21:40.990 STDERR Thread-2 [INFO] PANIC in rte_eal_init():
>  > 2017-06-13 11:21:40.990 STDERR Thread-2 [INFO] Cannot init memory
>  >
>  > I tried to disable ASLR, but it did not work.
>  >
>  > Does anyone have a similar situation?
>  > Based on this page (https://github.com/collectd/collectd/issues/2284), this
>  > is known issue and it seemed we can avoid this with "--base-virtaddr"
>  > option.
>  > Does anyone know how to use this option in ovs-dpdk?
>
>  You will need to use populate this option into other_config:dpdk-extra
>  in the ovsdb.
>
>ovs-vsctl --no-wait set Open_vSwitch \
>. other_config:dpdk-extra="--base-virtaddr=..."
>
>  I don't know the base virtual address value you should use, however.
>
>  -Aaron
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC PATCH 00/21] Add OVS DPDK keep-alive functionality

[Aaron Conole]

Bhanuprakash Bodireddy  writes:

> Keepalive feature is aimed at achieving Fastpath Service Assurance
> in OVS-DPDK deployments. It adds support for monitoring the packet
> processing cores(PMD thread cores) by dispatching heartbeats at regular
> intervals. Incase of heartbeat misses additional health checks are
> enabled on the PMD thread to detect the failure and the same shall be
> reported to higher level fault management systems/frameworks.
>
> The implementation uses OVSDB for reporting the datapath status and the
> health of the PMD threads. Any external monitoring application can read
> the status from OVSDB at regular intervals (or) subscribe to the updates
> in OVSDB so that they get notified when the changes happen on OVSDB.
>
> POSIX shared memory object is created and initialized for storing the
> status of the PMD threads. This is initialized by main thread(vswitchd)
> as part of init process and will be periodically updated by 'keepalive'
> thread. keepalive feature can be enabled through below OVSDB settings.
>
> enable-keepalive=true
>   - Keepalive feature is disabled by default.
>
> keepalive-interval="5000"
>   - Timer interval in milliseconds for monitoring the packet
> processing cores.
>
> keepalive-shm-name="/ovs_keepalive_shm_name"
>   - Shared memory block name where the events shall be updated.
>
> When KA is enabled, 'ovs-keepalive' thread shall be spawned that wakes
> up at regular intervals to update the timestamp and status of pmd cores
> in shared memory region. This information shall be read by vswitchd thread
> and write the status in to 'keepalive' column of Open_vSwitch table in OVSDB.
>
> An external monitoring framework like collectd with ovs events support
> can read (or) subscribe to the datapath status changes in ovsdb. When the 
> state
> is updated, the collectd shall be notified and will eventually relay the 
> status
> to ceilometer service running in the controller. Below is the high level
> overview of deployment model.
>
> Compute NodeControllerCompute Node
>
> Collectd  <--> Ceilometer <>   Collectd
>
> OvS DPDK   OvS DPDK
>
> +-+
> | VM  |
> +--+--+
> \---+---/
> |
> +--+---+   ++--+ +--+---+
> | OVS  |-> |   ovsevents plugin| --> |   collectd   |
> +--+---+   ++--+ +--+---+
>
> +--+-+ +---++ |
> | Ceilometer | <-- | collectd ceilometer plugin |  <---
> +--+-+ +---++
>
> Performance impact
> --
>   No noticeable performance or latency impact is observed with
>   KA feature enabled.
>
> Bhanuprakash Bodireddy (21):
>
> [10] Patches help update OVSDB with keepalive status
>
>   vswitch.xml: Add keepalive support.
>   ovsschema: Introduce 'keepalive' column in Open_vSwitch.
>   dpdk: Add helper functions for DPDK datapath keepalive.
>   process: Retrieve process status.
>   Keepalive: Add initial keepalive support.
>   bridge: Invoke keepalive framework.
>   keepalive: Add more helper functions to KA framework.
>   dpif-netdev: Register packet processing cores to KA framework.
>   dpif-netdev: Dispatch heartbeats for DPDK datapath.
>   keepalive: Retrieve PMD status periodically.
>   bridge: Update keepalive status in ovsdb
>
>   keepalive: Add support to query keepalive statistics.
>   keepalive: Add support to query keepalive status.
>   dpif-netdev: Add helper function to check false positives.
>
> [5] Following patches add additional health checks in case of heartbeat
> failure. The following can still be improved and WIP.
>
>   dpif-netdev: Add additional datapath health checks.
>   keepalive: Check the link status as part of PMD health checks.
>   keepalive: Check the packet statisitcs as part of PMD health checks.
>   keepalive: Check the PMD cycle stats as part of PMD health checks.
>   netdev-dpdk: Enable PMD health checks on heartbeat failure.
>
>   keepalive: Display extended Keepalive status.
>   Documentation: Update DPDK doc with Keepalive feature.
>

Hi Bhanu,

I've been playing with this a little bit;  is it too late to consider
tracking 'threads' instead of 'cores'?  I'm not sure what it means for a
particular core ID to be 'healthy' - but I know what 'pmd24' not
responding means.

Additionally, I'd suggest keeping words like 'healthy', and 'unhealthy'
out of it.  I'd basically just have this keepalive report things on the
thread you *know* - last time it poked your status register (and you can
also track things like cpu utilization, etc, if you'd like).  Then let
your higher level thing that reads ceilometer make those "healthy"
determinations.  After all, sometimes 0% utilization is 

Re: [ovs-dev] [PATCH] ovs-ofctl: New option "--no-stats" for "ovs-ofctl dump-flows".

[Aaron Conole]

Ben Pfaff <b...@ovn.org> writes:

> It's pretty common to want to omit statistics from output, to make it
> easier to read.  This commit adds an ovs-ofctl option to make that easy.
>
> A lot of the OVS internal tests could use this, too, in place of
> ofctl_strip.  This commit adopts it for a subset.
>
> CC: Aaron Conole <acon...@redhat.com>
> Signed-off-by: Ben Pfaff <b...@ovn.org>
> ---

LGTM.  Thanks for doing this, Ben!

Acked-by: Aaron Conole <acon...@redhat.com>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovs-ctl: allow gdb as a wrapper

[Aaron Conole]

Ben Pfaff <b...@ovn.org> writes:

> On Mon, Jun 12, 2017 at 04:39:51PM -0400, Aaron Conole wrote:
>> It has been useful to attach gdb to the running process.  With this commit
>> we make it a little easier, as the daemon will have a gdbserver process
>> attached and listening on a specific port.
>> 
>> Signed-off-by: Aaron Conole <acon...@redhat.com>
>
> Will you please document this in ovs-ctl.8?

Whoops.  Sure thing.

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

Re: [ovs-dev] [PATCH 01/31] userspace: Add OXM field MFF_PACKET_TYPE

[Aaron Conole]

Hi Ben,

Ben Pfaff  writes:

> From: Jan Scheurich 
>
> Allow packet type namespace OFPHTN_ETHERTYPE as alternative pre-requisite
> for matching L3 protocols (MPLS, IP, IPv6, ARP etc).
>
> Change the meta-flow definition of packet_type field to use the new
> custom format MFS_PACKET_TYPE representing "(NS,NS_TYPE)".
>
> Parsing routine for MFS_PACKET_TYPE added to meta-flow.c. Formatting
> routine for field packet_type extracted from match_format() and moved to
> flow.c to be used from meta-flow.c for formatting MFS_PACKET_TYPE.
>
> Updated the ovs-fields man page source meta-flow.xml with documentation
> for packet-type-aware bridges and added documentation for field packet_type.
>
> Added packet_type to the matching properties in tests/ofproto.at. Should be
> removed later, when packet_type_aware bridge attribute will be introduced.
>
> Signed-off-by: Jan Scheurich 
> Signed-off-by: Ben Pfaff 

Does this need a co-authored by?  If not, there's probably a useful
change from the checkpatch side to match From: tags with Signed-off-by:
tags (for these delivery cases to squelch the error message).

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

Re: [ovs-dev] rte_eal_init() error when using ovs-dpdk with secondary application.

[Aaron Conole]

Junguk Cho  writes:

> Hi,
>
> I use ovs-dpdk (ovs-2.7, dpdk-16.11.1) with one application which talks to
> ovs by using ring device and "--proc-type=secondary" (secondary processes).
> It generally works well, but sometimes it shows this error.
>
> It seems it could not find correct memory mapping in the application.
>
> 2017-06-13 11:21:40.988 STDERR Thread-2 [INFO] EAL: Probing VFIO support...
> 2017-06-13 11:21:40.988 STDERR Thread-2 [INFO] EAL: WARNING: Address Space
> Layout Randomization (ASLR) is enabled in the kernel.
> 2017-06-13 11:21:40.988 STDERR Thread-2 [INFO] EAL:This may cause
> issues with mapping memory into secondary processes
> 2017-06-13 11:21:40.989 STDERR Thread-2 [INFO] EAL: Could not mmap
> 17146314752 <(714)%20631-4752> bytes in /dev/zero at [0x7f662c80], got
> [0x7f650600] - please use '--base-virtaddr' option
> 2017-06-13 11:21:40.989 STDERR Thread-2 [INFO] EAL: It is recommended to
> disable ASLR in the kernel and retry running both primary and secondary
> processes
> 2017-06-13 11:21:40.990 STDERR Thread-2 [INFO] PANIC in rte_eal_init():
> 2017-06-13 11:21:40.990 STDERR Thread-2 [INFO] Cannot init memory
>
> I tried to disable ASLR, but it did not work.
>
> Does anyone have a similar situation?
> Based on this page (https://github.com/collectd/collectd/issues/2284), this
> is known issue and it seemed we can avoid this with "--base-virtaddr"
> option.
> Does anyone know how to use this option in ovs-dpdk?

You will need to use populate this option into other_config:dpdk-extra
in the ovsdb.

  ovs-vsctl --no-wait set Open_vSwitch \
  . other_config:dpdk-extra="--base-virtaddr=..."

I don't know the base virtual address value you should use, however.

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

[ovs-dev] [PATCH] redhat: make the rpm aware of the lock file

[Aaron Conole]

Currently, the db lockfile will cause the openvswitch directory to
linger after uninstall because the rpm database isn't aware that it
should be treated as part of the system.  This commit informs the rpmdb
properly as a 'ghost' so that when the package is uninstalled, it will
be removed automatically.  This means that if no extra files exist in
/etc/openvswitch, the whole directory will be removed from /etc/.

Acked-by: Flavio Leitner <f...@sysclose.org>
Reviewed-by: Markos Chandras <mchan...@suse.de>
Signed-off-by: Aaron Conole <acon...@redhat.com>
---
 rhel/openvswitch-fedora.spec.in | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
index 9fc5f27..f822ad3 100644
--- a/rhel/openvswitch-fedora.spec.in
+++ b/rhel/openvswitch-fedora.spec.in
@@ -273,6 +273,7 @@ rm -rf $RPM_BUILD_ROOT/%{_datadir}/openvswitch/python/
 install -d -m 0755 $RPM_BUILD_ROOT/%{_sharedstatedir}/openvswitch
 
 touch $RPM_BUILD_ROOT%{_sysconfdir}/openvswitch/conf.db
+touch $RPM_BUILD_ROOT%{_sysconfdir}/openvswitch/.conf.db.~lock~
 touch $RPM_BUILD_ROOT%{_sysconfdir}/openvswitch/system-id.conf
 
 install -p -m 644 -D selinux/openvswitch-custom.pp \
@@ -481,6 +482,7 @@ fi
 %{_sysconfdir}/bash_completion.d/ovs-vsctl-bashcomp.bash
 %dir %{_sysconfdir}/openvswitch
 %config %ghost %{_sysconfdir}/openvswitch/conf.db
+%ghost %{_sysconfdir}/openvswitch/.conf.db.~lock~
 %config %ghost %{_sysconfdir}/openvswitch/system-id.conf
 %config(noreplace) %{_sysconfdir}/sysconfig/openvswitch
 %config(noreplace) %{_sysconfdir}/logrotate.d/openvswitch
-- 
2.9.4

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

Re: [ovs-dev] [PATCH 2/6] redhat: replace python3 with python package macro

[Aaron Conole]

Aaron Conole <acon...@redhat.com> writes:

> Flavio Leitner <f...@sysclose.org> writes:
>
>> On Sat, Jun 03, 2017 at 11:09:57AM -0400, Aaron Conole wrote:
>>> According to the packaging guidelines found at
>>> https://fedoraproject.org/wiki/PackagingDrafts:Python3EPEL, when
>>> specifying a python3 package, use the %{python3_pkgversion} macro to get
>>> the appropriate suffix.
>>
>> This looks incomplete because the package's name remains python3-openvswitch
>> where it should have been python%{python3_pkgversion}-openvswitch.
>> Same issue with the requires for that subpackage.
>
> Makes sense.  I'll fix it up, and test.
>
> Thanks Flavio!
>

With a clean Fedora system, I don't see the error that I was getting
that led me to change this.

Additionally, some asking around has led me to understand
that those guidelines are for EPEL packages (openvswitch is not an EPEL
package).  And it seems like they may not even be current.

I'm going to drop this patch from my submission.  Sorry for the noise.

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

Re: [ovs-dev] [PATCH v2 1/2] install-doc: suggest to use ovs-ctl for start/stop

[Aaron Conole]

Ben Pfaff <b...@ovn.org> writes:

> On Mon, Apr 17, 2017 at 01:59:49PM -0400, Aaron Conole wrote:
>> The install documentation guided users to manually start/stop
>> daemons.  This is good information to have, but with the
>> existence of ovs-ctl, is probably not the best way to start
>> guiding new users of ovs.
>> 
>> Suggest that users start by running ovs-ctl start, and
>> document the ability to selectively start/stop the daemons.
>> The ovs-ctl script is already mentioned a bit in the install
>> doc, so this just reinforces its use.
>> 
>> Suggested-by: Ben Pfaff <b...@ovn.org>
>> Signed-off-by: Aaron Conole <acon...@redhat.com>
>> ---
>> v1->v2:
>> * Added the location of ovs-ctl script
>
> I had to fold the following in because of:

d'oh!  Sorry for that.

...

> and then I applied both of these to master.  Thank you!

Thanks, Ben.  You rock!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 0/8] checkpatch: enhancements

[Aaron Conole]

Ben Pfaff <b...@ovn.org> writes:

> On Wed, Apr 26, 2017 at 11:54:52AM -0400, Aaron Conole wrote:
>> The following series refactors checkpatch to make
>> file-type specific checks.  This lets checkpatch have
>> finer grained checks, and should reduce the amount
>> of false positives.
>> 
>> Additionally, I've incorporated an older request to print
>> files in the more standard `filename:lineno: ` form that
>> most editors support out of the box.  And while I was in
>> there, I added pretty colors, because we all need some
>> color in our terminals.
>> 
>> v2:
>> * Fix flake8 errors
>> * Remove python from the line-length errors blacklist
>
> Thanks a lot for working on checkpatch.  It's great to have more and
> better tools for keeping quality high.
>
> But I still get flake8 errors.
>
> Added in patch 1, fixed in patch 5:
> ../utilities/checkpatch.py:180:12: E999 SyntaxError: invalid syntax

D'oh!  That's from some silly c-habits.  Fixed.

> Added in patch 3, never fixed:
> ../utilities/checkpatch.py:202:14: E241 multiple spaces after ':'

Oops.  My flake8 setup had this warning turned off for some reason.
Will be corrected in v3.  Sorry.

> Thanks,

Thanks so much for the review, Ben!

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

[ovs-dev] [PATCH v3 4/7] checkpatch: correct a parsing issue

[Aaron Conole]

Occasionally, characters will be sent which violate the
ascii decoder's sense of propriety.  In fact, in-tree there are
a few such files (ex: tests/atlocal.in), and they cause an
exception to be raised when they are encountered.

Set the policy to ignore these cases.  This means these bytes are
omitted from the text stream during processing.

Signed-off-by: Aaron Conole <acon...@redhat.com>
---
 utilities/checkpatch.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 68c4156..2088865 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -252,7 +252,7 @@ def ovs_checkpatch_parse(text):
 is_co_author = re.compile(r'(\s*(Co-authored-by: )(.*))$',
   re.I | re.M | re.S)
 
-for line in text.decode().split('\n'):
+for line in text.decode(errors='ignore').split('\n'):
 if current_file != previous_file:
 previous_file = current_file
 
-- 
2.9.3

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

[ovs-dev] [PATCH v3 5/7] checkpatch: print conformance

[Aaron Conole]

Other utilities (notoriously the linux kernel's checkpatch.pl) have a more
standardized form for printing file and lines.  With this change, the
template used to print gains two enhancements:
1. Color
2. Conformance with the kernel's version of checkpatch.pl

Signed-off-by: Aaron Conole <acon...@redhat.com>
---
 utilities/checkpatch.py | 81 ++---
 1 file changed, 49 insertions(+), 32 deletions(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 2088865..6eb1e1f 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -23,33 +23,41 @@ __errors = 0
 __warnings = 0
 print_file_name = None
 checking_file = False
+total_line = 0
+colors = False
 
 
-def print_file():
-global print_file_name
-if print_file_name:
-print("In file %s" % print_file_name)
-print_file_name = None
+def get_color_end():
+global colors
+if colors:
+return "\033[00m"
+return ""
 
 
-def print_error(message, lineno=None):
+def get_red_begin():
+global colors
+if colors:
+return "\033[91m"
+return ""
+
+
+def get_yellow_begin():
+global colors
+if colors:
+return "\033[93m"
+return ""
+
+
+def print_error(message):
 global __errors
-print_file()
-if lineno is not None:
-print("E(%d): %s" % (lineno, message))
-else:
-print("E: %s" % (message))
+print("%sERROR%s: %s" % (get_red_begin(), get_color_end(), message))
 
 __errors = __errors + 1
 
 
-def print_warning(message, lineno=None):
+def print_warning(message):
 global __warnings
-print_file()
-if lineno:
-print("W(%d): %s" % (lineno, message))
-else:
-print("W: %s" % (message))
+print("%sWARNING%s: %s" % (get_yellow_begin(), get_color_end(), message))
 
 __warnings = __warnings + 1
 
@@ -176,33 +184,29 @@ checks = [
  'match_name':
  lambda x: not any([fmt in x for fmt in line_length_blacklist]),
  'check': lambda x: line_length_check(x),
- 'print':
- lambda(x): print_warning("Line is greater than 79-characters long", x)},
+ 'print': lambda: print_warning("Line length is >79-characters long")},
 
 {'regex': '$(?https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v4 1/7] checkpatch: introduce a flexible framework

[Aaron Conole]

Developers wishing to add checks to checkpatch sift through an adhoc mess,
currently.  The process goes something like:
1. Figure out what to test in the patch
2. Write some code, quickly, that checks for that condition
3. Look through the statemachine to find where the check should go
4. ignore parts of the above and just throw something together

That worked fine for the initial development, but as interesting new tests
are developed, it is important to have a more flexible framework that lets
a developer just plug in a new test, easily.

This commit brings in a new framework that allows plugging in checks very
quickly.  Hook up the line-length test as an initial demonstration.

Signed-off-by: Aaron Conole <acon...@redhat.com>
---
 utilities/checkpatch.py | 51 -
 1 file changed, 42 insertions(+), 9 deletions(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 638ac97..b3b833b 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -164,6 +164,47 @@ def pointer_whitespace_check(line):
 return __regex_ptr_declaration_missing_whitespace.search(line) is not None
 
 
+def line_length_check(line):
+"""Return TRUE if the line length is too long"""
+if len(line) > 79:
+return True
+return False
+
+
+checks = [
+{'regex': None,
+ 'match_name':
+ lambda x: not any([fmt in x for fmt in line_length_blacklist]),
+ 'check': lambda x: line_length_check(x),
+ 'print':
+ lambda x: print_warning("Line is greater than 79-characters long", x)}
+]
+
+
+def get_file_type_checks(filename):
+"""Returns the list of checks for a file based on matching the filename
+   against regex."""
+global checks
+checkList = []
+for check in checks:
+if check['regex'] is None and check['match_name'] is None:
+checkList.append(check)
+if check['regex'] is not None and \
+   re.compile(check['regex']).search(filename) is not None:
+checkList.append(check)
+elif check['match_name'] is not None and check['match_name'](filename):
+checkList.append(check)
+return checkList
+
+
+def run_checks(current_file, line, lineno):
+"""Runs the various checks for the particular line.  This will take
+   filename into account."""
+for check in get_file_type_checks(current_file):
+if check['check'](line):
+check['print'](lineno)
+
+
 def ovs_checkpatch_parse(text):
 global print_file_name
 lineno = 0
@@ -180,15 +221,10 @@ def ovs_checkpatch_parse(text):
   re.I | re.M | re.S)
 is_co_author = re.compile(r'(\s*(Co-authored-by: )(.*))$',
   re.I | re.M | re.S)
-skip_line_length_check = False
 
 for line in text.decode().split('\n'):
 if current_file != previous_file:
 previous_file = current_file
-if any([fmt in current_file for fmt in line_length_blacklist]):
-skip_line_length_check = True
-else:
-skip_line_length_check = False
 
 lineno = lineno + 1
 if len(line) <= 0:
@@ -250,13 +286,10 @@ def ovs_checkpatch_parse(text):
 print_line = True
 print_warning("Line has non-spaces leading whitespace",
   lineno)
+run_checks(current_file, cmp_line, lineno)
 if trailing_whitespace_or_crlf(cmp_line):
 print_line = True
 print_warning("Line has trailing whitespace", lineno)
-if len(cmp_line) > 79 and not skip_line_length_check:
-print_line = True
-print_warning("Line is greater than 79-characters long",
-  lineno)
 if not if_and_for_whitespace_checks(cmp_line):
 print_line = True
 print_error("Improper whitespace around control block",
-- 
2.9.3

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

[ovs-dev] [PATCH v4 0/7] checkpatch: enhancements

[Aaron Conole]

The following series refactors checkpatch to make
file-type specific checks.  This lets checkpatch have
finer grained checks, and should reduce the amount
of false positives.

Additionally, I've incorporated an older request to print
files in the more standard `filename:lineno: ` form that
most editors support out of the box.  And while I was in
there, I added pretty colors, because we all need some
color in our terminals.

v2:
* Fix flake8 errors
* Remove python from the line-length errors blacklist

v3:
* Fix the remaining flake8 errors (make flake8-check
  passes on my system)

v4:
* Fix a python 3 lambda syntax error.

Aaron Conole (7):
  checkpatch: introduce a flexible framework
  checkpatch: common print_line
  checkpatch: move the checks to the framework
  checkpatch: correct a parsing issue
  checkpatch: print conformance
  checkpatch: filename from hunks fix
  checkpatch: fix pointer declaration

 utilities/checkpatch.py | 166 
 1 file changed, 112 insertions(+), 54 deletions(-)

-- 
2.9.3

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

[ovs-dev] [PATCH v4 5/7] checkpatch: print conformance

[Aaron Conole]

Other utilities (notoriously the linux kernel's checkpatch.pl) have a more
standardized form for printing file and lines.  With this change, the
template used to print gains two enhancements:
1. Color
2. Conformance with the kernel's version of checkpatch.pl

Signed-off-by: Aaron Conole <acon...@redhat.com>
---
 utilities/checkpatch.py | 81 ++---
 1 file changed, 49 insertions(+), 32 deletions(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 9f8f30f..6eb1e1f 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -23,33 +23,41 @@ __errors = 0
 __warnings = 0
 print_file_name = None
 checking_file = False
+total_line = 0
+colors = False
 
 
-def print_file():
-global print_file_name
-if print_file_name:
-print("In file %s" % print_file_name)
-print_file_name = None
+def get_color_end():
+global colors
+if colors:
+return "\033[00m"
+return ""
 
 
-def print_error(message, lineno=None):
+def get_red_begin():
+global colors
+if colors:
+return "\033[91m"
+return ""
+
+
+def get_yellow_begin():
+global colors
+if colors:
+return "\033[93m"
+return ""
+
+
+def print_error(message):
 global __errors
-print_file()
-if lineno is not None:
-print("E(%d): %s" % (lineno, message))
-else:
-print("E: %s" % (message))
+print("%sERROR%s: %s" % (get_red_begin(), get_color_end(), message))
 
 __errors = __errors + 1
 
 
-def print_warning(message, lineno=None):
+def print_warning(message):
 global __warnings
-print_file()
-if lineno:
-print("W(%d): %s" % (lineno, message))
-else:
-print("W: %s" % (message))
+print("%sWARNING%s: %s" % (get_yellow_begin(), get_color_end(), message))
 
 __warnings = __warnings + 1
 
@@ -176,33 +184,29 @@ checks = [
  'match_name':
  lambda x: not any([fmt in x for fmt in line_length_blacklist]),
  'check': lambda x: line_length_check(x),
- 'print':
- lambda x: print_warning("Line is greater than 79-characters long", x)},
+ 'print': lambda: print_warning("Line length is >79-characters long")},
 
 {'regex': '$(?https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v4 4/7] checkpatch: correct a parsing issue

[Aaron Conole]

Occasionally, characters will be sent which violate the
ascii decoder's sense of propriety.  In fact, in-tree there are
a few such files (ex: tests/atlocal.in), and they cause an
exception to be raised when they are encountered.

Set the policy to ignore these cases.  This means these bytes are
omitted from the text stream during processing.

Signed-off-by: Aaron Conole <acon...@redhat.com>
---
 utilities/checkpatch.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 9d4e469..9f8f30f 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -252,7 +252,7 @@ def ovs_checkpatch_parse(text):
 is_co_author = re.compile(r'(\s*(Co-authored-by: )(.*))$',
   re.I | re.M | re.S)
 
-for line in text.decode().split('\n'):
+for line in text.decode(errors='ignore').split('\n'):
 if current_file != previous_file:
 previous_file = current_file
 
-- 
2.9.3

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

[ovs-dev] Cover letters (was Re: [PATCH 0/7] Add OVS DPDK keep-alive functionality)

[Aaron Conole]

Hi Ben,

Ben Pfaff  writes:
...
> I think it'd be even better to include measurements in one of the commit
> messages, because those are available in the repository after the
> patches are applied.  It's harder to find cover letters because they're
> only on the mailing list.

One thing that other projects do to keep the cover letters is to create
a branch, apply all the patches from the series, and then create a merge
commit, with the commit message as the cover letter.  It can make the
history a bit messier, but means that the cover letter is retained, and
makes 'git bisect' work quite a bit better (because bisect knows how to
skip merged trees appropriately).  It could be worth doing, because it
can help to explain an overall motivation for a series when walking the
tree with git annotations.

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

[ovs-dev] [PATCH v3 1/7] checkpatch: introduce a flexible framework

[Aaron Conole]

Developers wishing to add checks to checkpatch sift through an adhoc mess,
currently.  The process goes something like:
1. Figure out what to test in the patch
2. Write some code, quickly, that checks for that condition
3. Look through the statemachine to find where the check should go
4. ignore parts of the above and just throw something together

That worked fine for the initial development, but as interesting new tests
are developed, it is important to have a more flexible framework that lets
a developer just plug in a new test, easily.

This commit brings in a new framework that allows plugging in checks very
quickly.  Hook up the line-length test as an initial demonstration.

Signed-off-by: Aaron Conole <acon...@redhat.com>
---

 utilities/checkpatch.py | 51 -
 1 file changed, 42 insertions(+), 9 deletions(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 638ac97..4ee5670 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -164,6 +164,47 @@ def pointer_whitespace_check(line):
 return __regex_ptr_declaration_missing_whitespace.search(line) is not None
 
 
+def line_length_check(line):
+"""Return TRUE if the line length is too long"""
+if len(line) > 79:
+return True
+return False
+
+
+checks = [
+{'regex': None,
+ 'match_name':
+ lambda x: not any([fmt in x for fmt in line_length_blacklist]),
+ 'check': lambda x: line_length_check(x),
+ 'print':
+ lambda(x): print_warning("Line is greater than 79-characters long", x)}
+]
+
+
+def get_file_type_checks(filename):
+"""Returns the list of checks for a file based on matching the filename
+   against regex."""
+global checks
+checkList = []
+for check in checks:
+if check['regex'] is None and check['match_name'] is None:
+checkList.append(check)
+if check['regex'] is not None and \
+   re.compile(check['regex']).search(filename) is not None:
+checkList.append(check)
+elif check['match_name'] is not None and check['match_name'](filename):
+checkList.append(check)
+return checkList
+
+
+def run_checks(current_file, line, lineno):
+"""Runs the various checks for the particular line.  This will take
+   filename into account."""
+for check in get_file_type_checks(current_file):
+if check['check'](line):
+check['print'](lineno)
+
+
 def ovs_checkpatch_parse(text):
 global print_file_name
 lineno = 0
@@ -180,15 +221,10 @@ def ovs_checkpatch_parse(text):
   re.I | re.M | re.S)
 is_co_author = re.compile(r'(\s*(Co-authored-by: )(.*))$',
   re.I | re.M | re.S)
-skip_line_length_check = False
 
 for line in text.decode().split('\n'):
 if current_file != previous_file:
 previous_file = current_file
-if any([fmt in current_file for fmt in line_length_blacklist]):
-skip_line_length_check = True
-else:
-skip_line_length_check = False
 
 lineno = lineno + 1
 if len(line) <= 0:
@@ -250,13 +286,10 @@ def ovs_checkpatch_parse(text):
 print_line = True
 print_warning("Line has non-spaces leading whitespace",
   lineno)
+run_checks(current_file, cmp_line, lineno)
 if trailing_whitespace_or_crlf(cmp_line):
 print_line = True
 print_warning("Line has trailing whitespace", lineno)
-if len(cmp_line) > 79 and not skip_line_length_check:
-print_line = True
-print_warning("Line is greater than 79-characters long",
-  lineno)
 if not if_and_for_whitespace_checks(cmp_line):
 print_line = True
 print_error("Improper whitespace around control block",
-- 
2.9.3

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

[ovs-dev] [PATCH v3 2/7] checkpatch: common print_line

[Aaron Conole]

With the new framework, print_line can be moved out to the checks
framework.

Signed-off-by: Aaron Conole <acon...@redhat.com>
---
 utilities/checkpatch.py | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 4ee5670..239ce69 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -200,9 +200,14 @@ def get_file_type_checks(filename):
 def run_checks(current_file, line, lineno):
 """Runs the various checks for the particular line.  This will take
filename into account."""
+print_line = False
 for check in get_file_type_checks(current_file):
 if check['check'](line):
 check['print'](lineno)
+print_line = True
+
+if print_line:
+print("\n%s\n" % line)
 
 
 def ovs_checkpatch_parse(text):
@@ -258,7 +263,6 @@ def ovs_checkpatch_parse(text):
 m = is_co_author.match(line)
 co_authors.append(m.group(3))
 elif parse == 2:
-print_line = False
 newfile = hunks.match(line)
 if newfile:
 current_file = newfile.group(2)
@@ -283,26 +287,19 @@ def ovs_checkpatch_parse(text):
 continue
 if (not current_file.endswith('.mk') and
 not leading_whitespace_is_spaces(cmp_line)):
-print_line = True
 print_warning("Line has non-spaces leading whitespace",
   lineno)
 run_checks(current_file, cmp_line, lineno)
 if trailing_whitespace_or_crlf(cmp_line):
-print_line = True
 print_warning("Line has trailing whitespace", lineno)
 if not if_and_for_whitespace_checks(cmp_line):
-print_line = True
 print_error("Improper whitespace around control block",
 lineno)
 if not if_and_for_end_with_bracket_check(cmp_line):
-print_line = True
 print_error("Inappropriate bracing around statement", lineno)
 if pointer_whitespace_check(cmp_line):
-print_line = True
 print_error("Inappropriate spacing in pointer declaration",
 lineno)
-if print_line:
-print("\n%s\n" % line)
 if __errors or __warnings:
 return -1
 return 0
-- 
2.9.3

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

[ovs-dev] [PATCH v3 3/7] checkpatch: move the checks to the framework

[Aaron Conole]

All of the checks are now part of the new 'check' framework.

Signed-off-by: Aaron Conole <acon...@redhat.com>
---
 utilities/checkpatch.py | 41 ++---
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 239ce69..68c4156 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -177,7 +177,32 @@ checks = [
  lambda x: not any([fmt in x for fmt in line_length_blacklist]),
  'check': lambda x: line_length_check(x),
  'print':
- lambda(x): print_warning("Line is greater than 79-characters long", x)}
+ lambda(x): print_warning("Line is greater than 79-characters long", x)},
+
+{'regex': '$(?https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3 1/7] checkpatch: introduce a flexible framework

[Aaron Conole]

Ben Pfaff <b...@ovn.org> writes:

> On Mon, May 01, 2017 at 03:44:37PM -0400, Aaron Conole wrote:
>> Developers wishing to add checks to checkpatch sift through an adhoc mess,
>> currently.  The process goes something like:
>> 1. Figure out what to test in the patch
>> 2. Write some code, quickly, that checks for that condition
>> 3. Look through the statemachine to find where the check should go
>> 4. ignore parts of the above and just throw something together
>> 
>> That worked fine for the initial development, but as interesting new tests
>> are developed, it is important to have a more flexible framework that lets
>> a developer just plug in a new test, easily.
>> 
>> This commit brings in a new framework that allows plugging in checks very
>> quickly.  Hook up the line-length test as an initial demonstration.
>> 
>> Signed-off-by: Aaron Conole <acon...@redhat.com>
>
> With this, I get:
>
> ../utilities/checkpatch.py:180:12: E999 SyntaxError: invalid syntax
>
> where the cited line is:
>
>  lambda(x): print_warning("Line is greater than 79-characters long", x)}

d'oh!  Trying it with python3, I see:

File "./utilities/checkpatch.py", line 180
lambda(x): print_warning("Line is greater than 79-characters long", x)}
  ^
SyntaxError: invalid syntax

So, I guess that is it.  I'll submit a fix.  I think I broke my python
setup while doing some hacking on a different project, so I'll have to
be more careful.  Maybe now is the time to learn virtualenv.

> I'm using:
>
> $ flake8 --version
> 3.2.1 (mccabe: 0.5.3, pycodestyle: 2.2.0, pyflakes: 1.3.0) CPython 3.5.1+ 
> on Linux

Yep, that's different from my version:

$ flake8 --version
2.5.5 (pep8: 1.6.2, mccabe: 0.2.1, pyflakes: 1.2.3, hacking.core: 0.0.1, 
ProxyChecker: 0.0.1) CPython 2.7.13 on Linux

And I don't get that error flagged:

03:53:13 aconole {checkpatch_2} ~/git/ovs$ git show HEAD | grep checkpatch 
| head -n1
checkpatch: introduce a flexible framework
03:54:04 aconole {checkpatch_2} ~/git/ovs$ touch utilities/checkpatch.py 
03:54:20 aconole {checkpatch_2} ~/git/ovs$ make flake8-check
\
  src='Documentation/conf.py ofproto/ipfix-gen-entities 
utilities/ovs-pcap.in utilities/checkpatch.py utilities/ovs-dev.py 
utilities/ovs-tcpdump.in utilities/bugtool/ovs-bugtool.in tests/appctl.py 
tests/test-daemon.py tests/test-json.py tests/test-jsonrpc.py tests/test-l7.py 
tests/test-ovsdb.py tests/test-reconnect.py tests/MockXenAPI.py 
tests/test-unix-socket.py tests/test-unixctl.py tests/test-vlog.py 
xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync python/ovs/__init__.py 
python/ovs/daemon.py python/ovs/fcntl_win.py python/ovs/db/__init__.py 
python/ovs/db/data.py python/ovs/db/error.py python/ovs/db/idl.py 
python/ovs/db/parser.py python/ovs/db/schema.py python/ovs/db/types.py 
python/ovs/fatal_signal.py python/ovs/json.py python/ovs/jsonrpc.py 
python/ovs/ovsuuid.py python/ovs/poller.py python/ovs/process.py 
python/ovs/reconnect.py python/ovs/socket_util.py python/ovs/stream.py 
python/ovs/timeval.py python/ovs/unixctl/__init__.py 
python/ovs/unixctl/client.py python/o
 vs/unixctl/server.py python/ovs/util.py python/ovs/version.py 
python/ovs/vlog.py python/ovs/winutils.py python/ovstest/__init__.py 
python/ovstest/args.py python/ovstest/rpcserver.py python/ovstest/tcp.py 
python/ovstest/tests.py python/ovstest/udp.py python/ovstest/util.py 
python/ovstest/vswitch.py python/setup.py python/build/__init__.py 
python/build/nroff.py python/ovs/dirs.py.template vtep/ovs-vtep 
build-aux/xml2nroff' && \
  flake8 $src --select=H231,H232,H233,H238  && \
  flake8 $src 
--ignore=E121,E123,E125,E126,E127,E128,E129,E131,W503,F811,D,H,I  && \
  touch flake8-check
03:54:24 aconole {checkpatch_2} ~/git/ovs$ 

I'll look into fixing up my setup, but for now apologies for the
back-and-forth.


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

[ovs-dev] [PATCH v4 2/7] checkpatch: common print_line

[Aaron Conole]

With the new framework, print_line can be moved out to the checks
framework.

Signed-off-by: Aaron Conole <acon...@redhat.com>
---
 utilities/checkpatch.py | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index b3b833b..30c0a3e 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -200,9 +200,14 @@ def get_file_type_checks(filename):
 def run_checks(current_file, line, lineno):
 """Runs the various checks for the particular line.  This will take
filename into account."""
+print_line = False
 for check in get_file_type_checks(current_file):
 if check['check'](line):
 check['print'](lineno)
+print_line = True
+
+if print_line:
+print("\n%s\n" % line)
 
 
 def ovs_checkpatch_parse(text):
@@ -258,7 +263,6 @@ def ovs_checkpatch_parse(text):
 m = is_co_author.match(line)
 co_authors.append(m.group(3))
 elif parse == 2:
-print_line = False
 newfile = hunks.match(line)
 if newfile:
 current_file = newfile.group(2)
@@ -283,26 +287,19 @@ def ovs_checkpatch_parse(text):
 continue
 if (not current_file.endswith('.mk') and
 not leading_whitespace_is_spaces(cmp_line)):
-print_line = True
 print_warning("Line has non-spaces leading whitespace",
   lineno)
 run_checks(current_file, cmp_line, lineno)
 if trailing_whitespace_or_crlf(cmp_line):
-print_line = True
 print_warning("Line has trailing whitespace", lineno)
 if not if_and_for_whitespace_checks(cmp_line):
-print_line = True
 print_error("Improper whitespace around control block",
 lineno)
 if not if_and_for_end_with_bracket_check(cmp_line):
-print_line = True
 print_error("Inappropriate bracing around statement", lineno)
 if pointer_whitespace_check(cmp_line):
-print_line = True
 print_error("Inappropriate spacing in pointer declaration",
 lineno)
-if print_line:
-print("\n%s\n" % line)
 if __errors or __warnings:
 return -1
 return 0
-- 
2.9.3

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

[ovs-dev] [PATCH v4 3/7] checkpatch: move the checks to the framework

[Aaron Conole]

All of the checks are now part of the new 'check' framework.

Signed-off-by: Aaron Conole <acon...@redhat.com>
---
 utilities/checkpatch.py | 41 ++---
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 30c0a3e..9d4e469 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -177,7 +177,32 @@ checks = [
  lambda x: not any([fmt in x for fmt in line_length_blacklist]),
  'check': lambda x: line_length_check(x),
  'print':
- lambda x: print_warning("Line is greater than 79-characters long", x)}
+ lambda x: print_warning("Line is greater than 79-characters long", x)},
+
+{'regex': '$(?https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v4 7/7] checkpatch: fix pointer declaration

[Aaron Conole]

A common way of expressing 'raise to the power of' when authoring
comments uses **.  This is currently getting caught by the pointer
spacing warning.  So, catch it here.

Reported-by: Lance Richardson <lrich...@redhat.com>
Signed-off-by: Aaron Conole <acon...@redhat.com>
---
 utilities/checkpatch.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index acf6f15..f22ceac 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -76,7 +76,7 @@ __regex_is_for_if_single_line_bracket = \
 re.compile(r'^ +(if|for|while) \(.*\)')
 __regex_ends_with_bracket = \
 re.compile(r'[^\s]\) {(\s+/\*[\s\Sa-zA-Z0-9\.,\?\*/+-]*)?$')
-__regex_ptr_declaration_missing_whitespace = re.compile(r'[a-zA-Z0-9]\*')
+__regex_ptr_declaration_missing_whitespace = re.compile(r'[a-zA-Z0-9]\*[^*]')
 
 skip_leading_whitespace_check = False
 skip_trailing_whitespace_check = False
-- 
2.9.3

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

[ovs-dev] [PATCH v4 6/7] checkpatch: filename from hunks fix

[Aaron Conole]

Filenames that come from the hunks match include the git-ified 'b/'
prefix, which makes jumping to the error file that much harder.  This
patch corrects that by simply skipping those bytes.

Signed-off-by: Aaron Conole <acon...@redhat.com>
---
 utilities/checkpatch.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 6eb1e1f..acf6f15 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -300,7 +300,7 @@ def ovs_checkpatch_parse(text):
 elif parse == 2:
 newfile = hunks.match(line)
 if newfile:
-current_file = newfile.group(2)
+current_file = newfile.group(2)[2:]
 print_file_name = current_file
 continue
 reset_line_number = hunk_differences.match(line)
-- 
2.9.3

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

Re: [ovs-dev] [PATCH v4 7/7] checkpatch: fix pointer declaration

[Aaron Conole]

Ben Pfaff <b...@ovn.org> writes:

> On Mon, May 01, 2017 at 04:14:09PM -0400, Aaron Conole wrote:
>> A common way of expressing 'raise to the power of' when authoring
>> comments uses **.  This is currently getting caught by the pointer
>> spacing warning.  So, catch it here.
>> 
>> Reported-by: Lance Richardson <lrich...@redhat.com>
>> Signed-off-by: Aaron Conole <acon...@redhat.com>
>
> Thanks a lot for improving checkpatch, it should be helpful for
> review.

Thank you for the incrementals, and review.

> Maybe I'll start using it in my review process.

I use it by default in all of my development.  The following is my
.git/hooks/pre-commit:

  #!/bin/sh
  if git rev-parse --verify HEAD 2>/dev/null
  then
  git diff-index -p --cached HEAD
  else
  :
  fi | utilities/checkpatch.py -s

> I applied all of these patches to master.

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

[ovs-dev] [PATCH v3 0/7] checkpatch: enhancements

[Aaron Conole]

The following series refactors checkpatch to make
file-type specific checks.  This lets checkpatch have
finer grained checks, and should reduce the amount
of false positives.

Additionally, I've incorporated an older request to print
files in the more standard `filename:lineno: ` form that
most editors support out of the box.  And while I was in
there, I added pretty colors, because we all need some
color in our terminals.

v2:
* Fix flake8 errors
* Remove python from the line-length errors blacklist

v3:
* Fix the remaining flake8 errors (make flake8-check
  passes on my system)

Aaron Conole (7):
  checkpatch: introduce a flexible framework
  checkpatch: common print_line
  checkpatch: move the checks to the framework
  checkpatch: correct a parsing issue
  checkpatch: print conformance
  checkpatch: filename from hunks fix
  checkpatch: fix pointer declaration

 utilities/checkpatch.py | 166 
 1 file changed, 112 insertions(+), 54 deletions(-)

-- 
2.9.3

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

[ovs-dev] [PATCH v3 7/7] checkpatch: fix pointer declaration

[Aaron Conole]

A common way of expressing 'raise to the power of' when authoring
comments uses **.  This is currently getting caught by the pointer
spacing warning.  So, catch it here.

Reported-by: Lance Richardson <lrich...@redhat.com>
Signed-off-by: Aaron Conole <acon...@redhat.com>
---
 utilities/checkpatch.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index acf6f15..f22ceac 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -76,7 +76,7 @@ __regex_is_for_if_single_line_bracket = \
 re.compile(r'^ +(if|for|while) \(.*\)')
 __regex_ends_with_bracket = \
 re.compile(r'[^\s]\) {(\s+/\*[\s\Sa-zA-Z0-9\.,\?\*/+-]*)?$')
-__regex_ptr_declaration_missing_whitespace = re.compile(r'[a-zA-Z0-9]\*')
+__regex_ptr_declaration_missing_whitespace = re.compile(r'[a-zA-Z0-9]\*[^*]')
 
 skip_leading_whitespace_check = False
 skip_trailing_whitespace_check = False
-- 
2.9.3

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

[ovs-dev] [PATCH v3 6/7] checkpatch: filename from hunks fix

[Aaron Conole]

Filenames that come from the hunks match include the git-ified 'b/'
prefix, which makes jumping to the error file that much harder.  This
patch corrects that by simply skipping those bytes.

Signed-off-by: Aaron Conole <acon...@redhat.com>
---
 utilities/checkpatch.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 6eb1e1f..acf6f15 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -300,7 +300,7 @@ def ovs_checkpatch_parse(text):
 elif parse == 2:
 newfile = hunks.match(line)
 if newfile:
-current_file = newfile.group(2)
+current_file = newfile.group(2)[2:]
 print_file_name = current_file
 continue
 reset_line_number = hunk_differences.match(line)
-- 
2.9.3

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

[ovs-dev] [PATCH] rhel: fix the fedora spec

[Aaron Conole]

When commit d0c961a99f57 ("lib/automake.mk: don't install
runtime directories") landed, it broke RPM based builds since
the requisite directories were no longer available.  This commit
adds those directories back when making RPMs so that the package
manager can see them.

Fixes: d0c961a99f57 ("lib/automake.mk: don't install runtime directories")
Reported-by: Lance Richardson <lrich...@redhat.com>
Tested-by: Lance Richardson <lrich...@redhat.com>
Signed-off-by: Aaron Conole <acon...@redhat.com>
---
NOTE: If there is a required corresponding change
  for Debian, please let me know.

 rhel/openvswitch-fedora.spec.in | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
index 5d53284..6f67413 100644
--- a/rhel/openvswitch-fedora.spec.in
+++ b/rhel/openvswitch-fedora.spec.in
@@ -230,6 +230,8 @@ make -f %{_datadir}/selinux/devel/Makefile
 rm -rf $RPM_BUILD_ROOT
 make install DESTDIR=$RPM_BUILD_ROOT
 
+install -d -m 0755 $RPM_BUILD_ROOT%{_rundir}/openvswitch
+install -d -m 0755 $RPM_BUILD_ROOT%{_localstatedir}/log/openvswitch
 install -d -m 0755 $RPM_BUILD_ROOT%{_sysconfdir}/openvswitch
 
 install -p -D -m 0644 \
-- 
2.9.3

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

Re: [ovs-dev] [PATCH 2/2] rhel: delete transient ports on boot when starting ovsdb-server

[Aaron Conole]

Timothy Redaelli  writes:

> Use ovs-ctl --delete-transient-ports-on-boot to start ovsdb-server.
>
> This feature can be disabled by appending --no-delete-transient-ports-on-boot
> to OPTIONS in /etc/sysconfig/openvswitch
>
> Signed-off-by: Timothy Redaelli 
> ---
>  rhel/automake.mk | 1 +
>  rhel/openvswitch-fedora.spec.in  | 6 +-
>  rhel/usr_lib_systemd_system_ovsdb-server.service | 3 +--
>  rhel/usr_lib_tmpfiles.d_openvswitch.conf | 1 +
>  4 files changed, 8 insertions(+), 3 deletions(-)
>  create mode 100644 rhel/usr_lib_tmpfiles.d_openvswitch.conf
>
> diff --git a/rhel/automake.mk b/rhel/automake.mk
> index 1265fa74..cc24a0bc 100644
> --- a/rhel/automake.mk
> +++ b/rhel/automake.mk
> @@ -31,6 +31,7 @@ EXTRA_DIST += \
>   rhel/usr_lib_systemd_system_ovn-controller.service \
>   rhel/usr_lib_systemd_system_ovn-controller-vtep.service \
>   rhel/usr_lib_systemd_system_ovn-northd.service \
> + rhel/usr_lib_tmpfiles.d_openvswitch.conf \
>   rhel/usr_lib_firewalld_services_ovn-central-firewall-service.xml \
>   rhel/usr_lib_firewalld_services_ovn-host-firewall-service.xml
>  
> diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
> index 3200040e..3352bde7 100644
> --- a/rhel/openvswitch-fedora.spec.in
> +++ b/rhel/openvswitch-fedora.spec.in
> @@ -246,6 +246,9 @@ done
>  install -m 0755 rhel/etc_init.d_openvswitch \
>  $RPM_BUILD_ROOT%{_datadir}/openvswitch/scripts/openvswitch.init
>  
> +install -p -D -m 0644 rhel/usr_lib_tmpfiles.d_openvswitch.conf \
> +$RPM_BUILD_ROOT%{_tmpfilesdir}/openvswitch.conf
> +
>  install -p -D -m 0644 rhel/etc_logrotate.d_openvswitch \
>  $RPM_BUILD_ROOT/%{_sysconfdir}/logrotate.d/openvswitch
>  
> @@ -488,6 +491,7 @@ fi
>  %{_unitdir}/ovsdb-server.service
>  %{_unitdir}/ovs-vswitchd.service
>  %{_datadir}/openvswitch/scripts/openvswitch.init
> +%{_tmpfilesdir}/openvswitch.conf
>  %{_sysconfdir}/sysconfig/network-scripts/ifup-ovs
>  %{_sysconfdir}/sysconfig/network-scripts/ifdown-ovs
>  %{_datadir}/openvswitch/bugtool-plugins/
> @@ -534,7 +538,7 @@ fi
>  %doc COPYING NOTICE README.rst NEWS rhel/README.RHEL.rst
>  /var/lib/openvswitch
>  /var/log/openvswitch
> -%ghost %attr(755,root,root) %{_rundir}/openvswitch
> +%dir %{_rundir}/openvswitch

Doesn't this still need to be flagged as %ghost?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] checkpatch: Fix inconsistencies skipping datapath files.

[Aaron Conole]

Ben Pfaff <b...@ovn.org> writes:

> The code in checkpatch inconsistently stripped "a/" or "b/" from the
> beginning of a file name, and the check for "datapath" only worked when
> the prefix was not stripped.  This fixes the problem.
>
> CC: Aaron Conole <acon...@redhat.com>
> Signed-off-by: Ben Pfaff <b...@ovn.org>
> ---

LGTM - just tested it out.

Acked-by: Aaron Conole <acon...@redhat.com>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] RFC: ovs-dump-flows utility

[Aaron Conole]

Hi Ben,

Thanks for the look!

Ben Pfaff <b...@ovn.org> writes:

> On Fri, Apr 28, 2017 at 04:44:32PM -0400, Aaron Conole wrote:
>> Greetings dev,
>> 
>> I have whipped up a quick little utility (find below), that I've done a
>> bit of debugging with and it seems to have made working with dump-flows
>> from ovs-ofctl a little easier to use.
>> 
>> If you think it's worthwhile to add to the repository, I'll submit it
>> formally.  We were using it while debugging some strange packet
>> forwarding in openshift.
>> 
>> -Aaron
>
> Thanks for working to make the ovs-ofctl formatting better!
>
> I prefer to interpret this script as a kind of "feature request" for
> "ovs-ofctl dump-flows".  This command already has some special support,
> compared to other ovs-ofctl commands, and it might make sense to
> continue adding to it.

In which way?  It calls the same ofp-print code, iirc.

> ovs-ofctl dump-flows already has one of the features that this script
> adds, that is, sorting the flows and removing "OFPST_FLOW" lines.  You
> turn this on by using the "--sort" (or "--rsort") option.

Ahh, cool - I missed that.

> The other features that this script provides all seem like ones that
> would be useful to add to ovs-ofctl itself.  I'd tend to prefer to
> continue enhancing it rather than adding wrapper scripts; I think that
> this is likely to yield a more coherent design in the end, and possibly
> higher quality.  Is that something you're willing to consider?

I had started to work on this, back in December, but there were hundreds
of lines of existing formatting code that would have to change (this is
related to the discussion here:
https://mail.openvswitch.org/pipermail/ovs-dev/2016-December/326560.html),
and I thought it might be a better use of time to simply wrap the output
- especially since I didn't know if any future changes in that area were
going to happen.  The last thing I want to do is break the existing
output (which I do use quite a bit for debugging, so retraining myself
would be painful) if someone has scripts which rely on it.
Additionally, quite a few print commands would have changed to give the
data to the table structure, rather than a long string.  It looked to be
a rather large change for something that could be resolved with a
wrapper.  Maybe I misinterpreted your response (from here
https://mail.openvswitch.org/pipermail/ovs-dev/2016-December/326201.html).

The other thing that adds complication is replacing the port numbers
with names from the database.  That would require a second transaction
(unless there's a way to batch that during the initial dump-flows
request, but I couldn't see an obvious way), and I didn't know if it
would be okay to do (and how to treat failures... after all, it's
convenient, but it isn't requisite to have the numbers replaced with
names).  There are a few minor changes I have to my copy of the script
(I've added back the packet counts, and I have the port output in a way
that we can not-quite paste the flow back in to an add-flow), but I
ended up also using the direct output of dump-flows.

As for how to implement it, I could have put some kind of post processor
that would split the strings up (the way I have done with the script),
but that felt rather hacky (since it's basically the formatting script,
but in c-code form).

Anyway, I submitted this as a start.  If you think it's better to do the
work in the ofp-print library then I can revisit it.  Maybe the reduced
set of things that were really helpful, and the rest we can just say
"don't fear sed".

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

Re: [ovs-dev] checkpatch name checking

[Aaron Conole]

Ben Pfaff  writes:

> Hi Aaron, checkpatch currently tries to ignores files in the "datapath"
> directories but it's not entirely successful.  I think that's because,
> in the "parse == 1" case, it doesn't strip a leading "a/" or "b/" from
> filenames:
> current_file = match.group(2)
> whereas in the "parse == 2", it does:
> current_file = newfile.group(2)[2:]
> and the check for "datapath" relies on the / being there:
> # Skip files which have /datapath in them, since they are
> # linux or windows coding standards
> if '/datapath' in current_file:
> continue
> I'm not sure where the real bug is.  Should the prefix be consistently
> stripped or not stripped?  Once that's decided, it's easy to fix the
> problem.

I think it should be consistently stripped.  The regex matches don't
care, but when the filename is printed to the screen, it will include b/
which makes simply opening the file in an editor not work correctly.

I think given that, the parse==1 case is incorrect, and the datapath
check is also incorrect.

Make sense?

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

Re: [ovs-dev] [PATCH v5 0/6] ovn: Add IPv6 Router Solicitation responder support

[Aaron Conole]

Hi Numan,

nusid...@redhat.com writes:

> From: Numan Siddique 
>
> I have picked up the patch series authored by Zong Kai LI to support IPv6
> Router Advertisement in ovn -
> https://mail.openvswitch.org/pipermail/ovs-dev/2016-October/242988.html
> as this patch series was inactive for quite a long time.

Running checkpatch utility flags a number of line-length issues, and
some Signed-off-by / Co-authored-by signatures missing.

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

Re: [ovs-dev] Cover letters (was Re: [PATCH 0/7] Add OVS DPDK keep-alive functionality)

[Aaron Conole]

Ben Pfaff <b...@ovn.org> writes:

> On Mon, May 01, 2017 at 04:45:56PM -0400, Aaron Conole wrote:
>> Ben Pfaff <b...@ovn.org> writes:
>> ...
>> > I think it'd be even better to include measurements in one of the commit
>> > messages, because those are available in the repository after the
>> > patches are applied.  It's harder to find cover letters because they're
>> > only on the mailing list.
>> 
>> One thing that other projects do to keep the cover letters is to create
>> a branch, apply all the patches from the series, and then create a merge
>> commit, with the commit message as the cover letter.  It can make the
>> history a bit messier, but means that the cover letter is retained, and
>> makes 'git bisect' work quite a bit better (because bisect knows how to
>> skip merged trees appropriately).  It could be worth doing, because it
>> can help to explain an overall motivation for a series when walking the
>> tree with git annotations.
>
> I wouldn't object to that, for well-organized series.
>
> Is there a way to automate applying a branch like this from email?  Or
> is it better to do them as pull requests (whether via email or Github)?

I think patchwork provides access to the cover letter somehow.  I've
CC'd Stephen because he has a bit of experience with PW, and may be able
to comment on whether or not I'm just crazy.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v4 7/7] checkpatch: fix pointer declaration

[Aaron Conole]

Ben Pfaff <b...@ovn.org> writes:

> On Mon, May 01, 2017 at 04:39:30PM -0400, Aaron Conole wrote:
>> Ben Pfaff <b...@ovn.org> writes:
>> 
>> > On Mon, May 01, 2017 at 04:14:09PM -0400, Aaron Conole wrote:
>> >> A common way of expressing 'raise to the power of' when authoring
>> >> comments uses **.  This is currently getting caught by the pointer
>> >> spacing warning.  So, catch it here.
>> >> 
>> >> Reported-by: Lance Richardson <lrich...@redhat.com>
>> >> Signed-off-by: Aaron Conole <acon...@redhat.com>
>> >
>> > Thanks a lot for improving checkpatch, it should be helpful for
>> > review.
>> 
>> Thank you for the incrementals, and review.
>> 
>> > Maybe I'll start using it in my review process.
>> 
>> I use it by default in all of my development.  The following is my
>> .git/hooks/pre-commit:
>> 
>>   #!/bin/sh
>>   if git rev-parse --verify HEAD 2>/dev/null
>>   then
>>   git diff-index -p --cached HEAD
>>   else
>>   :
>>   fi | utilities/checkpatch.py -s
>> 
>> > I applied all of these patches to master.
>
> Oh, interesting.
>
> I'm playing with it by applying the following and then changing my
> "mutt" shortcut for applying a patch from
> cd ~/nicira/ovs && git am -s
> to
> cd ~/nicira/ovs && checkpatch.py -p | git am -s
>
> I'm not sure whether my patch makes sense, but it makes me feel clever
> either way.

:)  If it works - I don't know what the erroring output will do to the
git am input parser, but my guess is blow up (which is probably what you
want anyway).  I looked for solutions that use a git hook, but it seems
they all blow up whenever anything more trivial happens, so there's
probably not yet a slick solution.  With that said,

Acked-by: Aaron Conole <acon...@redhat.com>

> --8<--cut here-->8--
>
> From: Ben Pfaff <b...@ovn.org>
> Date: Mon, 1 May 2017 13:42:46 -0700
> Subject: [PATCH] checkpatch: Add support for "pass-through" mode.
>
> Signed-off-by: Ben Pfaff <b...@ovn.org>
> ---
>  utilities/checkpatch.py | 17 ++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 387549afe3f6..ced5e9f8c241 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -342,6 +342,8 @@ def usage():
>"Do not emit an error if no Signed-off-by line is present")
>  print("-t|--skip-trailing-whitespace\t"
>"Skips the trailing whitespace test")
> +print("-p|--pass-through\t"
> +  "Print a copy of the input as output, when reading from stdin")
>  
>  
>  def ovs_checkpatch_file(filename):
> @@ -366,17 +368,19 @@ def ovs_checkpatch_file(filename):
>  
>  if __name__ == '__main__':
>  try:
> -optlist, args = getopt.getopt(sys.argv[1:], 'bhlstf',
> +optlist, args = getopt.getopt(sys.argv[1:], 'bhlstfp',
>["check-file",
> "help",
> "skip-block-whitespace",
> "skip-leading-whitespace",
> "skip-signoff-lines",
> -   "skip-trailing-whitespace"])
> +   "skip-trailing-whitespace",
> +   "pass-through"])
>  except:
>  print("Unknown option encountered. Please rerun with -h for help.")
>  sys.exit(-1)
>  
> +pass_through = False
>  for o, a in optlist:
>  if o in ("-h", "--help"):
>  usage()
> @@ -391,6 +395,8 @@ if __name__ == '__main__':
>  skip_trailing_whitespace_check = True
>  elif o in ("-f", "--check-file"):
>  checking_file = True
> +elif o in ("-p", "--pass-through"):
> +pass_through = True
>  else:
>  print("Unknown option '%s'" % o)
>  sys.exit(-1)
> @@ -404,5 +410,10 @@ if __name__ == '__main__':
>  if sys.stdin.isatty():
>  usage()
>  sys.exit(-1)
> -sys.exit(ovs_checkpatch_parse(sys.stdin.read()))
> +
> +content = sys.stdin.read()
> +exit_code = ovs_checkpatch_parse(content)
> +if pass_through:
> +sys.stdout.write(content)
> +sys.exit(exit_code)
>  sys.exit(ovs_checkpatch_file(filename))
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] Support accepting and displaying port names in OVS tools.

[Aaron Conole]

Hi Ben,

Ben Pfaff <b...@ovn.org> writes:

> Until now, most ovs-ofctl commands have not accepted names for ports, only
> numbers, and have not been able to display port names either.  It's a lot
> easier for users if they can use and see meaningful names instead of
> arbitrary numbers.  This commit adds that support.
>
> For backward compatibility, only interactive ovs-ofctl commands by default
> display port names; to display them in scripts, use the new --names
> option.
>
> Signed-off-by: Ben Pfaff <b...@ovn.org>
> ---

Tested-by: Aaron Conole <acon...@redhat.com>

I suggest folding in the following (otherwise, I'm happy with it):

diff --git a/lib/dpctl.c b/lib/dpctl.c
index 1ebf5da..7f87603 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017 
Nicira, Inc.
+ * Copyright (c) 2008-2017 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv2] checkpatch: Check for stdlib usage.

[Aaron Conole]

Joe Stringer <j...@ovn.org> writes:

> Many standard library functions are wrapped in OVS, so check for usage
> of the original versions and suggest that authors replace them with the
> OVS versions.
>
> Signed-off-by: Joe Stringer <j...@ovn.org>
> ---
> v2: Drop checks for functions that don't replace library functions
> Fix naming of functions xfoo() -> ovs_foo() where appropriate
> Fix descriptions
> ---

This version LGTM.

Acked-by: Aaron Conole <acon...@redhat.com>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] MTU in i40e dpdk driver

[Aaron Conole]

Nitin Katiyar  writes:

> Hi,
> We are using OVS-DPDK (2.6 version) with Fortville NIC (configured in
> 25G mode) being used as dpdk port. The setup involves 2 VMs running on
> 2 different computes (destination VM in compute with 10G NIC while
> originating VM is in compute with Fortville NIC). All the interfaces
> in the path are configured with MTU of 2140.
>
> While pinging with size of 2112 (IP packet of 2140 bytes) we found
> that ping response does not reach originating VM (i.e on compute with
> Fortville NIC) . DPDK interface does not show any drop but we don't
> see any ping response received at DPDK port (verified using
> port-mirroring). We also don't see any rule in ovs dpctl for ping
> response. If we increase the MTU of DPDK interface by 4 bytes or
> reduce the ping size by 4 bytes then it works.
>
> The same configuration works between 10G NICs on both sides.
>
> Is it a known issue with i40 dpdk driver?

There are some issues with Fortville nics;  notably, that the ports
share some register values.  Have you tried your ovs+dpdk setup with the
following patch applied:

http://dpdk.org/ml/archives/dev/2017-August/072758.html

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

Re: [ovs-dev] ovs-tcpdump error

[Aaron Conole]

Hi Bhanu,

"Bodireddy, Bhanuprakash"  writes:

> Hi,
>
>  
>
> ovs-tcpdump throws the below error when trying to capture packets on one of 
> the
> vhostuserports.
>
>  
>
> $ ovs-tcpdump -i dpdkvhostuser0
>
>ERROR: Please create an interface called `midpdkvhostuser0`
>
> See your OS guide for how to do this.
>
> Ex: ip link add midpdkvhostuser0 type veth peer name midpdkvhostuser02
>
>  
>
> $ ip link add midpdkvhostuser0 type veth peer name midpdkvhostuser02
>
>  Error: argument "midpdkvhostuser0" is wrong: "name" too long
>
>  
>
> To get around this issue, I have to pass  ‘—mirror-to’ option as below.
>
>  
>
> $ ovs-tcpdump -i dpdkvhostuser0 -XX --mirror-to vh0
>
>  
>
> Is this due to the length of the port name?  Would be nice to fix this issue.

Thanks for the detailed write up.

It is related to the mirror port name length.  The mirror port is bound
by IFNAMSIZ restriction, so it must be 15 characters + nul, and
midpdkvhostuser0 would be 16 + nul.  This is a linux specific
restriction, and it won't be changed because it is very much a well
established UAPI (and changing it will have implications on code not
able to deal with larger sized name buffers).

I'm not sure how best to fix it.  My concession was the mirror-to
option.  Perhaps there's a better way?

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

Re: [ovs-dev] [PATCH v4 3/7] dpif-netdev: Register packet processing cores to KA framework.

[Aaron Conole]

"Bodireddy, Bhanuprakash"  writes:

>>"Bodireddy, Bhanuprakash"  writes:
>>
Bhanuprakash Bodireddy  writes:

> This commit registers the packet processing PMD cores to keepalive
> framework. Only PMDs that have rxqs mapped will be registered and
> actively monitored by KA framework.
>
> This commit spawns a keepalive thread that will dispatch heartbeats
> to PMD cores. The pmd threads respond to heartbeats by marking
> themselves alive. As long as PMD responds to heartbeats it is considered
>>'healthy'.
>
> Signed-off-by: Bhanuprakash Bodireddy
> 
> ---

I'm really confused with this patch.  I've stopped reviewing the series.

It seems like there's a mix of 'track by core id' and 'track by thread id'.

I don't think it's possible to do anything by core id.  We can never
know what else has been scheduled on those cores, and we cannot be
sure that a taskset or other scheduler provisioning call will move the
>>threads.
>>>
>>> [BHANU] I have already answered this in other thread.
>>> one can't correlate threads with cores and we shouldn't be tracking by
>>> cores. However with PMD threads there is 1:1 mapping of PMD and the
>>> core-id and it's safe to temporarily write PMD liveness info in to an
>>> array indexed by core id before updating this in to HMAP.
>>
>>The core-id as a concept here is deceptive.  An external entity (such as
>>taskset) can rebalance the PMDs.  External entities can be scheduled on the
>>cores.  I think it's dangerous to have anything called core-id in this series 
>>or
>>feature, because people will naturally infer things which aren't true.
>>Additionally, it will lead to things like "well, we know that core x,y,z are
>>running at A%, so we can schedule things thusly..."
>>
>>Makes sense?
>>
>
> The concerns above makes sense and you have a valid point. 
> Unfortunately the logic that you see w.r.t PMD, core_id mapping is something 
> that was
> implemented in rte_keepalive library and I inherited it. As the 1:1
> mapping of a thread(PMD)
> to core is deceptive and makes little sense, I reworked on a different
> approach with no impact
> on datapath performance. I was testing this last few days to check for
> perf impacts and other
> possible issues.
>
> Previous design:
> 
> As part of heartbeat mechanism (dispath_heartbeats()),  in keepalive_info 
> structure
> we had arrays indexed by core-ids used by PMDs and Keepalive thread for 
> heart-beating.
> The arrays were used to keep the logic simple and lock-free.
>
> Also Keepalive thread was updating the status periodically in to
> 'process_list' map using callback function.
>
> New design:
> ---
> we already have a 'process_list' map where all the PMD threads are
> added by main(vswitchd)
> thread. In this new approach, I take a copy of the 'process_list',
> let's call it 'cached_process_list'
> and use this cached map for heartbeating between Keepalive and PMD
> threads. No locks are
> needed on the 'cached_process_list' there by not impacting the datapath 
> performance.
>
> Also whenever there is datapath reconfiguration(triggered by
> pmd-cpu-mask), the 'process_list' map
> will be updated and also the cached_process_list will be reloaded from
> the main map there by having
> the maps in sync.  This is handled as part of
> ka_register_datapath_threads().  I have been testing this
> and seems to be working fine.
>
> This way we can completely avoid all references to core_id in this
> series. Let me know if you have
> any comments on this new approach.

Sounds good to me.  Looking forward to the code :-)

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

[ovs-dev] [PATCH] rhel: fix log directory permissions

[Aaron Conole]

When the logrotate script runs, and Open vSwitch is running as a non-root
user, the /var/log/openvswitch directory doesn't have other rx bits set.
This means the reopen attempt will fail with "permission denied", even though
the default logrotate configuration creates a new log file with the
appropriate attributes.

This change sets the r/x bits for other on /var/log/messages

Signed-off-by: Aaron Conole <acon...@redhat.com>
Tested-by: Jean Hsiao <jhs...@redhat.com>
---
 rhel/openvswitch-fedora.spec.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
index dd79fa9..8d62393 100644
--- a/rhel/openvswitch-fedora.spec.in
+++ b/rhel/openvswitch-fedora.spec.in
@@ -577,7 +577,7 @@ fi
 %endif
 %doc COPYING NOTICE README.rst NEWS rhel/README.RHEL.rst
 /var/lib/openvswitch
-/var/log/openvswitch
+%attr(755,-,-) /var/log/openvswitch
 %ghost %attr(755,root,root) %{_rundir}/openvswitch
 
 %files ovn-docker
-- 
2.9.4

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

Re: [ovs-dev] [PATCH v2] checkpatch: Reset line counter.

[Aaron Conole]

Ilya Maximets <i.maxim...@samsung.com> writes:

> Lines should be counted for each file separately.
>
> Signed-off-by: Ilya Maximets <i.maxim...@samsung.com>
> ---

Looks good to me.  Thanks, Ilya!

Acked-by: Aaron Conole <acon...@redhat.com>

> Version 2:
>   * Fixed commit message.
>
>  utilities/checkpatch.py | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 185ddaf..33feb6b 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -64,10 +64,11 @@ def print_warning(message):
>  
>  
>  def reset_counters():
> -global __errors, __warnings
> +global __errors, __warnings, total_line
>  
>  __errors = 0
>  __warnings = 0
> +total_line = 0
>  
>  
>  # These are keywords whose names are normally followed by a space and
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] unixctl: Give better error message for unknown commands.

[Aaron Conole]

Ben Pfaff <b...@ovn.org> writes:

> Signed-off-by: Ben Pfaff <b...@ovn.org>
> ---

LGTM.

Acked-by: Aaron Conole <acon...@redhat.com>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v4 3/7] dpif-netdev: Register packet processing cores to KA framework.

[Aaron Conole]

"Bodireddy, Bhanuprakash"  writes:

>>Bhanuprakash Bodireddy  writes:
>>
>>> This commit registers the packet processing PMD cores to keepalive
>>> framework. Only PMDs that have rxqs mapped will be registered and
>>> actively monitored by KA framework.
>>>
>>> This commit spawns a keepalive thread that will dispatch heartbeats to
>>> PMD cores. The pmd threads respond to heartbeats by marking themselves
>>> alive. As long as PMD responds to heartbeats it is considered 'healthy'.
>>>
>>> Signed-off-by: Bhanuprakash Bodireddy
>>> 
>>> ---
>>
>>I'm really confused with this patch.  I've stopped reviewing the series.
>>
>>It seems like there's a mix of 'track by core id' and 'track by thread id'.
>>
>>I don't think it's possible to do anything by core id.  We can never know what
>>else has been scheduled on those cores, and we cannot be sure that a taskset
>>or other scheduler provisioning call will move the threads.
>
> [BHANU] I have already answered this in other thread. 
> one can't correlate threads with cores and we shouldn't be tracking by
> cores. However with PMD threads
> there is 1:1 mapping of PMD and the core-id and it's safe to
> temporarily write PMD liveness info in to an array indexed
> by core id before updating this in to HMAP. 

The core-id as a concept here is deceptive.  An external entity (such as
taskset) can rebalance the PMDs.  External entities can be scheduled on
the cores.  I think it's dangerous to have anything called core-id
in this series or feature, because people will naturally infer things
which aren't true.  Additionally, it will lead to things like "well, we
know that core x,y,z are running at A%, so we can schedule things
thusly..."

Makes sense?

> However as already mentioned, we are using tid for all other purposes
> as it is unique across the system.
>
>>
>>>  lib/dpif-netdev.c |  70 +
>>>  lib/keepalive.c   | 153
>>++
>>>  lib/keepalive.h   |  17 ++
>>>  lib/util.c|  23 
>>>  lib/util.h|   2 +
>>>  5 files changed, 254 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
>>> e2cd931..84c7ffd 100644
>>> --- a/lib/dpif-netdev.c
>>> +++ b/lib/dpif-netdev.c
>>> @@ -49,6 +49,7 @@
>>>  #include "flow.h"
>>>  #include "hmapx.h"
>>>  #include "id-pool.h"
>>> +#include "keepalive.h"
>>>  #include "latch.h"
>>>  #include "netdev.h"
>>>  #include "netdev-vport.h"
>>> @@ -978,6 +979,63 @@ sorted_poll_thread_list(struct dp_netdev *dp,
>>>  *n = k;
>>>  }
>>>
>>> +static void *
>>> +ovs_keepalive(void *f_ OVS_UNUSED)
>>> +{
>>> +pthread_detach(pthread_self());
>>> +
>>> +for (;;) {
>>> +xusleep(get_ka_interval() * 1000);
>>> +}
>>> +
>>> +return NULL;
>>> +}
>>> +
>>> +static void
>>> +ka_thread_start(struct dp_netdev *dp) {
>>> +static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>>> +
>>> +if (ovsthread_once_start()) {
>>> +ovs_thread_create("ovs_keepalive", ovs_keepalive, dp);
>>> +
>>> +ovsthread_once_done();
>>> +}
>>> +}
>>> +
>>> +static void
>>> +ka_register_datapath_threads(struct dp_netdev *dp) {
>>> +int ka_init = get_ka_init_status();
>>> +VLOG_DBG("Keepalive: Was initialization successful? [%s]",
>>> +ka_init ? "Success" : "Failure");
>>> +if (!ka_init) {
>>> +return;
>>> +}
>>> +
>>> +ka_thread_start(dp);
>>> +
>>> +struct dp_netdev_pmd_thread *pmd;
>>> +CMAP_FOR_EACH (pmd, node, >poll_threads) {
>>> +/*  Register only PMD threads. */
>>> +if (pmd->core_id != NON_PMD_CORE_ID) {
>>> +int tid = ka_get_pmd_tid(pmd->core_id);
>>> +
>>> +/* Skip PMD thread with no rxqs mapping. */
>>
>>why skip these pmds?  we should still watch them, and then we can
>>correlated interesting events (for instance... when an rxq gets added whats
>>the change in utilization, etc).
>
> [BHANU]  We shoud skip the PMDs that has no rxqs mapped. This would happen in 
> cases
> where there are more PMD threads than the number of rxqs. 
>
> More importantly a PMD thread with no mapped rxq will not even enter
> the receive loop and
> will be in sleep state as below. 

Sure - that's okay.  It's consistent.

> $ ps -eLo tid,psr,comm,state | grep pmd
>  51727   3 pmd61   R
>  51747   0 pmd62   S
>  51749   1 pmd63   S
>  51750   2 pmd64   R
>  51756   6 pmd65   S
>  51758   7 pmd66   R
>  51759   4 pmd67   R
>  51760   5 pmd68   S
>
> When an rxq gets added to a sleeping PMD thread, the datapath reconfiguration 
> happens,
> this time threads get registered to KA framework as below.
>
> CP:  reconfigure_datapath() -> ka_register_datapath_threads() -> 
> ka_register_thread().
>
>>
>>> +if 

Re: [ovs-dev] [PATCH] redhat: fix upgrades where group doesn't exist

[Aaron Conole]

Aaron Conole <acon...@redhat.com> writes:

> The upgrade from older Open vSwitch versions on RHEL will try, as much as
> possible, to preserve the system.  This means no new users or groups are
> created.  As an effect, it's possible for the chown to fail, because the
> hugetlbfs group may not exist.  While it did on my systems, it was not
> there on others.
>
> This change allows the ExecStartPre commands to fail.  In the case that the
> user doesn't use DPDK, it won't matter anyway.
>
> Fixes: e3e738a3d058 ('redhat: allow dpdk to also run as non-root user')
> Signed-off-by: Aaron Conole <acon...@redhat.com>
> Reported-by: Jean-Tsung Hsiao <jhs...@redhat.com>
> Tested-by: Jean-Tsung Hsiao <jhs...@redhat.com>
> ---

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

Re: [ovs-dev] [PATCH v4 2/7] Keepalive: Add initial keepalive support.

[Aaron Conole]

Hi Bhanu,

Bhanuprakash Bodireddy  writes:

> This commit introduces the initial keepalive support by adding
> 'keepalive' module and also helper and initialization functions
> that will be invoked by later commits.
>
> This commit adds new ovsdb column "keepalive" that shows the status
> of the datapath threads. This is implemented for DPDK datapath and
> only status of PMD threads is reported.

I don't see the value in having this enabled / disabled flag?  Why not just
always have it on?

Additionally, even setting these true in this commit won't do anything.
No tracking starts until 3/7, afaict.

I guess it's okay to document here, but it might be worth stating that.

> For eg:
>   To enable keepalive feature.
>   'ovs-vsctl --no-wait set Open_vSwitch . other_config:enable-keepalive=true'

I'm not sure that a separate enable / disable flag is needed.

>   To set timer interval of 5000ms for monitoring packet processing cores.
>   'ovs-vsctl --no-wait set Open_vSwitch . \
>  other_config:keepalive-interval="5000"
>
> Signed-off-by: Bhanuprakash Bodireddy 
> ---

As stated earlier, please add a Documentation/ update with this.

>  lib/automake.mk|   2 +
>  lib/keepalive.c| 183 
> +
>  lib/keepalive.h|  87 +
>  vswitchd/bridge.c  |   3 +
>  vswitchd/vswitch.ovsschema |   8 +-
>  vswitchd/vswitch.xml   |  49 
>  6 files changed, 330 insertions(+), 2 deletions(-)
>  create mode 100644 lib/keepalive.c
>  create mode 100644 lib/keepalive.h
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 2415f4c..0d99f0a 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -110,6 +110,8 @@ lib_libopenvswitch_la_SOURCES = \
>   lib/json.c \
>   lib/jsonrpc.c \
>   lib/jsonrpc.h \
> + lib/keepalive.c \
> + lib/keepalive.h \
>   lib/lacp.c \
>   lib/lacp.h \
>   lib/latch.h \
> diff --git a/lib/keepalive.c b/lib/keepalive.c
> new file mode 100644
> index 000..ac73a42
> --- /dev/null
> +++ b/lib/keepalive.c
> @@ -0,0 +1,183 @@
> +/*
> + * Copyright (c) 2014, 2015, 2016, 2017 Nicira, Inc.

This line is not appropriately attributing the file.

> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "keepalive.h"
> +#include "lib/vswitch-idl.h"
> +#include "openvswitch/vlog.h"
> +#include "timeval.h"
> +
> +VLOG_DEFINE_THIS_MODULE(keepalive);
> +
> +static bool keepalive_enable = false;/* Keepalive disabled by default */
> +static bool ka_init_status = ka_init_failure; /* Keepalive initialization */

You're assigning this bool a value from an enum.  I know that's probably
allowed, but it looks strange to me.  I would prefer that this type
either reflect the enum type or a true/false value is used instead.

> +static uint32_t keepalive_timer_interval; /* keepalive timer interval */
> +static struct keepalive_info *ka_info = NULL;

Why allocate ka_info?  It will simplify some of the later code to just
keep it statically available.  It also means you can eliminate the
xzalloc() and free() calls you use later on in code.

Also, the nice thing about a static declaration is the structure will
already be 0 filled, and you'll know at program initialization time
whether it will succeed in getting the allocation.

> +
> +inline bool

The inline keyword is inappropriate in .c files.  Please let the
compiler do it's job.

> +ka_is_enabled(void)
> +{
> +return keepalive_enable;
> +}
> +

I'm not sure about enable / disable.  In this case, I think the branches
are not needed at all.  Better to have this always enabled and just deal
with whatever fallout may come.

> +inline int

See previous comment about inline and c files.

> +ka_get_pmd_tid(unsigned core_idx)
> +{
> +if (ka_is_enabled()) {
> +return ka_info->thread_id[core_idx];
> +}
> +
> +return -EINVAL;

I would expect EINVAL for core_idx which addresses outside of the
thread_id range.

Actually, I'm wondering why have a "core-id" based lookup at all?  Keep
a map of tid which are being monitored.  tid will never change; core_idx
can be manipulated by outside forces (taskset comes to mind).

I'm not sure there's a useful way of correlating core / task usage.  If
the task can be moved by the 

Re: [ovs-dev] [PATCH v4 3/7] dpif-netdev: Register packet processing cores to KA framework.

[Aaron Conole]

Bhanuprakash Bodireddy  writes:

> This commit registers the packet processing PMD cores to keepalive
> framework. Only PMDs that have rxqs mapped will be registered and
> actively monitored by KA framework.
>
> This commit spawns a keepalive thread that will dispatch heartbeats to
> PMD cores. The pmd threads respond to heartbeats by marking themselves
> alive. As long as PMD responds to heartbeats it is considered 'healthy'.
>
> Signed-off-by: Bhanuprakash Bodireddy 
> ---

I'm really confused with this patch.  I've stopped reviewing the series.

It seems like there's a mix of 'track by core id' and 'track by thread
id'.

I don't think it's possible to do anything by core id.  We can never
know what else has been scheduled on those cores, and we cannot be sure
that a taskset or other scheduler provisioning call will move the
threads.

>  lib/dpif-netdev.c |  70 +
>  lib/keepalive.c   | 153 
> ++
>  lib/keepalive.h   |  17 ++
>  lib/util.c|  23 
>  lib/util.h|   2 +
>  5 files changed, 254 insertions(+), 11 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index e2cd931..84c7ffd 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -49,6 +49,7 @@
>  #include "flow.h"
>  #include "hmapx.h"
>  #include "id-pool.h"
> +#include "keepalive.h"
>  #include "latch.h"
>  #include "netdev.h"
>  #include "netdev-vport.h"
> @@ -978,6 +979,63 @@ sorted_poll_thread_list(struct dp_netdev *dp,
>  *n = k;
>  }
>  
> +static void *
> +ovs_keepalive(void *f_ OVS_UNUSED)
> +{
> +pthread_detach(pthread_self());
> +
> +for (;;) {
> +xusleep(get_ka_interval() * 1000);
> +}
> +
> +return NULL;
> +}
> +
> +static void
> +ka_thread_start(struct dp_netdev *dp)
> +{
> +static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> +
> +if (ovsthread_once_start()) {
> +ovs_thread_create("ovs_keepalive", ovs_keepalive, dp);
> +
> +ovsthread_once_done();
> +}
> +}
> +
> +static void
> +ka_register_datapath_threads(struct dp_netdev *dp)
> +{
> +int ka_init = get_ka_init_status();
> +VLOG_DBG("Keepalive: Was initialization successful? [%s]",
> +ka_init ? "Success" : "Failure");
> +if (!ka_init) {
> +return;
> +}
> +
> +ka_thread_start(dp);
> +
> +struct dp_netdev_pmd_thread *pmd;
> +CMAP_FOR_EACH (pmd, node, >poll_threads) {
> +/*  Register only PMD threads. */
> +if (pmd->core_id != NON_PMD_CORE_ID) {
> +int tid = ka_get_pmd_tid(pmd->core_id);
> +
> +/* Skip PMD thread with no rxqs mapping. */

why skip these pmds?  we should still watch them, and then we can
correlated interesting events (for instance... when an rxq gets added
whats the change in utilization, etc).

> +if (OVS_UNLIKELY(!hmap_count(>poll_list))) {
> +/* rxq mapping changes due to reconfiguration,
> + * if there are no rxqs mapped to PMD, unregister it. */
> +ka_unregister_thread(tid, true);
> +continue;
> +}
> +
> +ka_register_thread(tid, true);
> +VLOG_INFO("Registered PMD thread [%d] on Core [%d] to KA 
> framework",
> +  tid, pmd->core_id);
> +}
> +}
> +}
> +
>  static void
>  dpif_netdev_pmd_info(struct unixctl_conn *conn, int argc, const char *argv[],
>   void *aux)
> @@ -3625,6 +3683,9 @@ reconfigure_datapath(struct dp_netdev *dp)
>  
>  /* Reload affected pmd threads. */
>  reload_affected_pmds(dp);
> +
> +/* Register datapath threads to KA monitoring. */
> +ka_register_datapath_threads(dp);
>  }
>  
>  /* Returns true if one of the netdevs in 'dp' requires a reconfiguration */
> @@ -3824,6 +3885,8 @@ pmd_thread_main(void *f_)
>  
>  poll_list = NULL;
>  
> +ka_store_pmd_id(pmd->core_id);
> +
>  /* Stores the pmd thread's 'pmd' to 'per_pmd_key'. */
>  ovsthread_setspecific(pmd->dp->per_pmd_key, pmd);
>  ovs_numa_thread_setaffinity_core(pmd->core_id);
> @@ -3859,6 +3922,9 @@ reload:
>: PMD_CYCLES_IDLE);
>  }
>  
> +/* Mark PMD thread alive. */
> +ka_mark_pmd_thread_alive(pmd->core_id);
> +
>  if (lc++ > 1024) {
>  bool reload;
>  
> @@ -3892,6 +3958,10 @@ reload:
>  }
>  
>  emc_cache_uninit(>flow_cache);
> +
> +int tid = ka_get_pmd_tid(pmd->core_id);
> +ka_unregister_thread(tid, true);
> +
>  free(poll_list);
>  pmd_free_cached_ports(pmd);
>  return NULL;
> diff --git a/lib/keepalive.c b/lib/keepalive.c
> index ac73a42..dfafaeb 100644
> --- a/lib/keepalive.c
> +++ b/lib/keepalive.c
> @@ -23,6 +23,7 @@
>  #include "keepalive.h"
>  #include "lib/vswitch-idl.h"
>  #include "openvswitch/vlog.h"
> 

Re: [ovs-dev] [PATCH v4 0/7] Add OVS DPDK keep-alive functionality.

[Aaron Conole]

"Bodireddy, Bhanuprakash"  writes:

> Hi Aaron,
>
>>Quick comment before I do an in-depth review.
>>
>>One thing that is missing in this series is some form of documentation added
>>to explain why this feature should exist (for instance, why can't the standard
>>posix process accounting information suffice?) and what the high-level
>>concepts are (you have the states being used, but I don't see a definition 
>>that
>>will be needed to understand when reading a keep-alive report).
>
> I am planning to write a cookbook with instructions on setting up Keepalive 
> in OvS, 
> Installing & configuring collectd and setting up ceilometer service to read 
> the events.
> The definition of the KA states and how to interpret them would be explained 
> in the
> document. Also the minimal step guide would be added in to OvS-DPDK Advanced 
> guide
> with links to cookbook. 

Please put that as you go in the patches.  It will make review easier,
too.

> On your other question on why posix process accounting isn't enough, please 
> see below
> for testcase and details.
>
>>
>>I think there could be a reason to provide this, but I think it's important to
>>explain why collectd will need to use the ovsdb interface, rather than calling
>>ex: times[1] or parsing /proc//stat for the runtime (and watching
>>accumulation).
>
> 1) On collectd reading ovsdb rather than directly monitoring the threads.
>
>   Collectd for sure is one popular daemon to collect and monitor system 
> wide statistics.
>   However, if we move ovs thread monitoring functionality to collectd we 
> are *forcing*
>   the users to use collectd to monitor OvS health. This may not be a 
> problem for someone using
>   collectd + OpenStack. 

It's important to note - collectd monitoring threads has nothing to do
with this feature.  If collectd can monitor threads from arbitrary
processes and report, it becomes much more powerful, no?  Let's keep it
focused on Open vSwitch.

>   Think of customer using OvS but having their proprietary monitoring 
> application with OpenStack or
>   worse their own orchestrator, in this case it's easy for them to 
> monitor OvS by querying OvSDB
>   with minimal code changes in to their app. 
>
>   Also it might be easy for any monitoring application to query/subscribe 
> to OvSDB for knowing the
>   OvS configuration and health. 

I don't really like using the idea of proprietary monitors as
justification for this.

OTOH, I think there's a good justification when it comes to multi-node
Open vSwitch tracking.  There, it may not be possible to aggregate the
statistics on each individual node (due to possible some kind of
administration policy) - so I agree having something like this exposed
through ovsdb could be useful.

> 2) On /proc/[pid]/stats:
>
> - I do read 'stats' file in 01/7  patch to get the thread name and 'core id' 
> the thread was last scheduled.
> - The other fields related to time in stats file can't be completely relied 
> due to below test case.
> 
> This test scenario was to simulate & identify the PMD stalls when a higher 
> priority thread(kernel/other IO thread) 
> gets scheduled on the same core.
>
> Test scenario:
> - OvS with single/multiple PMD thread.
> - Start a worker thread spinning continuously on the core (stress -c 1 &).
> - Change the worker thread attributes to RT (chrt -r -p 99  ).
> - Pin the worker thread on the same core as one of the PMDs  (taskset -p 
>  )
>
> Now the PMD stalls as the other worker thread has higher priority and is 
> favored & scheduled by Linux scheduler preempting PMD thread.
> However the /proc/pid/stat shows that the thread is still in 
>  *Running (R)* state ->   field 3 
>  (see the output below)
>Utime,stime were incrementing  ->field 14, 15(-do-)
>
> All the other time related fields were '0' as they don't apply here.
> For fields information:  http://man7.org/linux/man-pages/man5/proc.5.html
>
> ---sample 
> output---
> $ cat /proc/12506/stat
> 12506 (pmd61) R 1 12436 12436 0 -1 4210752 101244 0 0 0 389393 3099 0 0 20 0 
> 35 0 226680879 4798472192 4363 18446744073709551615
>  4194304 9786556 140737290674320 140344414947088 4467454 0 0 4096 24579 0 0 0 
> -1 3 0 0 0 0 0 11883752 12196256 48676864 140737290679638
>  140737290679790 140737290679790 140737290682316 0
> 
>
> But with the KA framework, the PMD stall be detected immediately and
> reported.

Please make sure this kind of tests and detection is detailed in the
documentation.  It really does help to understand the justification.

That said, I suspect that when those times were increasing, they really
were getting some system / user time.  Was your worker thread
occasionally making 

Re: [ovs-dev] [PATCH v4 0/7] Add OVS DPDK keep-alive functionality.

[Aaron Conole]

Hi Bhanu,

Bhanuprakash Bodireddy  writes:

> Keepalive feature is aimed at achieving Fastpath Service Assurance
> in OVS-DPDK deployments. It adds support for monitoring the packet
> processing cores(PMD thread cores) by dispatching heartbeats at regular
> intervals. Incase of heartbeat misses additional health checks are
> enabled on the PMD thread to detect the failure and the same shall be
> reported to higher level fault management systems/frameworks.
>
> The implementation uses OVSDB for reporting the health of the PMD threads.
> Any external monitoring application can read the status from OVSDB at 
> regular intervals (or) subscribe to the updates in OVSDB so that they get
> notified when the changes happen on OVSDB.
>
> keepalive info struct is created and initialized for storing the
> status of the PMD threads. This is initialized by main thread(vswitchd)
> as part of init process and will be periodically updated by 'keepalive'
> thread. keepalive feature can be enabled through below OVSDB settings.
>
> enable-keepalive=true
>   - Keepalive feature is disabled by default.
>
> keepalive-interval="5000"
>   - Timer interval in milliseconds for monitoring the packet
> processing cores.
>
> When KA is enabled, 'ovs-keepalive' thread shall be spawned that wakes
> up at regular intervals to update the timestamp and status of pmd cores
> in keepalive info struct. This information shall be read by vswitchd thread
> and write the status in to 'keepalive' column of Open_vSwitch table in OVSDB.
>
> An external monitoring framework like collectd with ovs events support
> can read (or) subscribe to the datapath status changes in ovsdb. When the 
> state
> is updated, the collectd shall be notified and will eventually relay the 
> status
> to ceilometer service running in the controller. Below is the high level
> overview of deployment model.
>
> Compute NodeControllerCompute Node
>
> Collectd  <--> Ceilometer <>   Collectd
>
> OvS DPDK   OvS DPDK
>
> +-+
> | VM  |
> +--+--+
> \---+---/
> |
> +--+---+   ++--+ +--+---+
> | OVS  |-> |   ovsevents plugin| --> |   collectd   |
> +--+---+   ++--+ +--+---+
>
> +--+-+ +---++ |
> | Ceilometer | <-- | collectd ceilometer plugin |  <---
> +--+-+ +---++
>
> github: The patches can be found here:
>   https://github.com/bbodired/ovs (Last master commit e7cd8c363)
>
> Performance impact:
>   No noticeable performance or latency impact is observed with
>   KA feature enabled.
>
> -

Quick comment before I do an in-depth review.

One thing that is missing in this series is some form of documentation
added to explain why this feature should exist (for instance, why can't
the standard posix process accounting information suffice?) and what the
high-level concepts are (you have the states being used, but I don't see
a definition that will be needed to understand when reading a keep-alive
report).

I think there could be a reason to provide this, but I think it's
important to explain why collectd will need to use the ovsdb interface,
rather than calling ex: times[1] or parsing /proc//stat for the
runtime (and watching accumulation).

Without that, it's difficult to evaluate the relative usefulness of
this.  I know I did ask for some of this documentation back in April,
and some was added for your larger series.  However, I think it's
important to add it as you go instead of a large plunk of it at the
end.  It really does help to understand why the feature should exist.

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

Re: [ovs-dev] [PATCH] rhel: fix log directory permissions

[Aaron Conole]

Flavio Leitner <f...@sysclose.org> writes:

> On Fri, 22 Sep 2017 09:44:18 -0400
> Aaron Conole <acon...@redhat.com> wrote:
>
>> When the logrotate script runs, and Open vSwitch is running as a non-root
>> user, the /var/log/openvswitch directory doesn't have other rx bits set.
>> This means the reopen attempt will fail with "permission denied", even though
>> the default logrotate configuration creates a new log file with the
>> appropriate attributes.
>> 
>> This change sets the r/x bits for other on /var/log/messages
>
> /var/log/openvswitch? :-)

D'oh!  Let's blame it on the problem between the keyboard and chair.

Russell - since you're likely the committer for this, do you want a v2
with a fixed message, or would you be able to fix it during apply?

> Reproduced here
> # ovs-appctl -t ovs-vswitchd vlog/reopen 
> Permission denied
> ovs-appctl: ovs-vswitchd: server returned an error
>
> Acked-by: Flavio Leitner <f...@sysclose.org>
>
>
>> 
>> Signed-off-by: Aaron Conole <acon...@redhat.com>
>> Tested-by: Jean Hsiao <jhs...@redhat.com>
>> ---
>>  rhel/openvswitch-fedora.spec.in | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/rhel/openvswitch-fedora.spec.in 
>> b/rhel/openvswitch-fedora.spec.in
>> index dd79fa9..8d62393 100644
>> --- a/rhel/openvswitch-fedora.spec.in
>> +++ b/rhel/openvswitch-fedora.spec.in
>> @@ -577,7 +577,7 @@ fi
>>  %endif
>>  %doc COPYING NOTICE README.rst NEWS rhel/README.RHEL.rst
>>  /var/lib/openvswitch
>> -/var/log/openvswitch
>> +%attr(755,-,-) /var/log/openvswitch
>>  %ghost %attr(755,root,root) %{_rundir}/openvswitch
>>  
>>  %files ovn-docker
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv2] checkpatch: Enforce bracing around conditionals.

[Aaron Conole]

Joe Stringer <j...@ovn.org> writes:

> The coding style states that BSD-style brace placement should be used,
> and even single statements should be enclosed. Add checks to checkpatch
> for this, particularly for 'else' statements.
>
> Signed-off-by: Joe Stringer <j...@ovn.org>
> ---

Acked-by: Aaron Conole <acon...@redhat.com>


Interestingly - if I do:

  $ find lib/ -name \*.c -exec ./utilities/checkpatch.py -f {} \; | \
 grep bracing | wc -l

before this patch: 92 instances of 'Inappropriate bracing'
after this patch: 102 instances of 'Inappropriate bracing'

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

Re: [ovs-dev] [PATCH v3 2/2] rhel: delete transient ports on boot when starting ovsdb-server

[Aaron Conole]

"Timothy M. Redaelli" <tredae...@redhat.com> writes:

> On 08/11/2017 07:12 PM, Aaron Conole wrote:
>> Timothy Redaelli <tredae...@redhat.com> writes:
>> 
>>> Use ovs-ctl --delete-transient-ports-on-boot to start ovsdb-server.
>>>
>>> This feature can be disabled by appending 
>>> --no-delete-transient-ports-on-boot
>>> to OPTIONS in /etc/sysconfig/openvswitch
>>>
>>> Signed-off-by: Timothy Redaelli <tredae...@redhat.com>
>>> ---
>>>  rhel/usr_lib_systemd_system_ovsdb-server.service | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/rhel/usr_lib_systemd_system_ovsdb-server.service 
>>> b/rhel/usr_lib_systemd_system_ovsdb-server.service
>>> index 7acd25f78..42473161e 100644
>>> --- a/rhel/usr_lib_systemd_system_ovsdb-server.service
>>> +++ b/rhel/usr_lib_systemd_system_ovsdb-server.service
>>> @@ -10,14 +10,14 @@ Type=forking
>>>  Restart=on-failure
>>>  EnvironmentFile=/etc/openvswitch/default.conf
>>>  EnvironmentFile=-/etc/sysconfig/openvswitch
>>> +ExecStartPre=/usr/bin/mkdir -p /var/run/openvswitch
>>>  ExecStartPre=/usr/bin/chown ${OVS_USER_ID} /var/run/openvswitch
>>>  ExecStart=/usr/share/openvswitch/scripts/ovs-ctl \
>>>--no-ovs-vswitchd --no-monitor --system-id=random \
>>> +  --delete-transient-ports-on-boot \
>>>--ovs-user=${OVS_USER_ID} \
>>>start $OPTIONS
>>>  ExecStop=/usr/share/openvswitch/scripts/ovs-ctl --no-ovs-vswitchd stop
>>>  ExecReload=/usr/share/openvswitch/scripts/ovs-ctl --no-ovs-vswitchd \
>>> --ovs-user=${OVS_USER_ID} \
>>> --no-monitor restart $OPTIONS
>>> -RuntimeDirectory=openvswitch
>>> -RuntimeDirectoryMode=0755
>> 
>> I'm not sure about this change.  One thing that's nice is the way
>> systemd will cleanup the runtime directories when this is done.  I think
>> this can leave it around.
>> 
>> I realize that under rhel (and fedora), the /run mountpoint is tmpfs.
>> 
>> Is there another way we could accomodate this?  Does a db flag make
>> sense?
>
> I don't think we can use db since we need that flags to be reset on each
> reboot (that's the reason I use /run).

I *think* systemd has a way of tracking how many starts the service has
had since boot time.  If not, that might actually be a good feature to
add to systemd, and then pull in this service file (because then we
don't need a flag in the DB, just query the environment variable for the
counter and if it is the very first start, delete - otherwise don't).

I'm not sure.  I asked in the #systemd chat on freenode, and the answer
was something like it exists, but might not be exposed via environment
variable.

I realize that would make the approach take longer to implement, but it
also does feel like it puts the concerns in the correct layers (ie: the
thing that can track restarts does so and informs the thing that cares
about them).

What do you think?

> In the previous patchset this run directory was created by using
> tmpfiles.d, but unlucky tmpfiles.d cannot use environment files, so you
> can't use ${OVS_USER_ID}.

That's kind of a shame, but I understand.

> If cleanup is needed, but imho it should be done by ovs-ctl or
> ovs-vswitchd/ovsdb-server directly, we may do the cleanup on ExecStopPost.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 1/3] selinux: move chr_file to non-dpdk as well

[Aaron Conole]

A last-minute change to the selinux policy caught by testing
incorrectly omitted moving a definition from non-dpdk to dpdk.

This moves the chr_file definition to a non-dpdk enabled permission,
which should allow non-dpdk enabled builds to work.

Fixes: 84d272330506 ("selinux: update policy to reflect non-root and dpdk 
support")
Signed-off-by: Aaron Conole <acon...@redhat.com>
---
 selinux/openvswitch-custom.te.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/selinux/openvswitch-custom.te.in b/selinux/openvswitch-custom.te.in
index 853de16..c1a774f 100644
--- a/selinux/openvswitch-custom.te.in
+++ b/selinux/openvswitch-custom.te.in
@@ -18,6 +18,7 @@ require {
 @end_dpdk@
 
 class capability { dac_override audit_write };
+class chr_file { write getattr read open ioctl };
 class dir { write remove_name add_name lock read };
 class file { write getattr read open execute execute_no_trans create 
unlink };
 class netlink_audit_socket { create nlmsg_relay audit_write read write 
};
@@ -25,7 +26,6 @@ require {
 class unix_stream_socket { write getattr read connectto connect setopt 
getopt sendto accept bind recvfrom acceptfrom };
 
 @begin_dpdk@
-class chr_file { write getattr read open ioctl };
 class tun_socket { relabelfrom relabelto create };
 @end_dpdk@
 }
-- 
2.9.4

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

[ovs-dev] [PATCH 0/3] Address some fallout from the selinux patches

[Aaron Conole]

In haste to get this in, a few things were missed.

Patch one addresses a last-minute change that was embarassingly
not committed.

Patch two updates documentation (reported by Ansis)

Patch three addresses a centos rpmbuild issue (reported by Ansis)

Aaron Conole (3):
  selinux: move chr_file to non-dpdk as well
  selinux.rst: point to the correct file
  centos: fix selinux intermediate file

 Documentation/howto/selinux.rst  | 2 +-
 rhel/openvswitch.spec.in | 1 +
 selinux/.gitignore   | 1 +
 selinux/openvswitch-custom.te.in | 2 +-
 4 files changed, 4 insertions(+), 2 deletions(-)
 create mode 100644 selinux/.gitignore

-- 
2.9.4

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

[ovs-dev] [PATCH 3/3] centos: fix selinux intermediate file

[Aaron Conole]

The commit 7bc1aae71e89 ("rhel: make the selinux policy intermediate")
broke the centos RPM builds.  This commit ensures that the centos rpmbuild
will first create the openvswitch-custom.te file, and then create the
final policy files.

Fixes: 7bc1aae71e89 ("rhel: make the selinux policy intermediate")
Reported-by: Ansis Atteka <aatt...@ovn.org>
Signed-off-by: Aaron Conole <acon...@redhat.com>
---
 rhel/openvswitch.spec.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/rhel/openvswitch.spec.in b/rhel/openvswitch.spec.in
index 412f3cd..e510d35 100644
--- a/rhel/openvswitch.spec.in
+++ b/rhel/openvswitch.spec.in
@@ -68,6 +68,7 @@ Tailored Open vSwitch SELinux policy
 ./configure --prefix=/usr --sysconfdir=/etc --localstatedir=%{_localstatedir} \
 --libdir=%{_libdir} --enable-ssl --enable-shared
 make %{_smp_mflags}
+make selinux/openvswitch-custom.te
 cd selinux
 make -f %{_datadir}/selinux/devel/Makefile
 
-- 
2.9.4

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

Re: [ovs-dev] [PATCH v3 0/3] updated selinux policy for Open vSwitch

[Aaron Conole]

Ansis Atteka <ansisatt...@gmail.com> writes:

> On 31 August 2017 at 16:22, Aaron Conole <acon...@redhat.com> wrote:
>> This series brings about a policy update to openvswitch allowing it to
>> run on a RHEL / Fedora system, even as a non-root user, with selinux set
>> to Enforcing.
>>
>> The first two patches make some changes to the way the selinux policy is
>> built to have a macro-like effect, allowing the dpdk policy to be enabled
>> or disabled based on the build.  This is chosen instead of using an selinux
>> boolean, because it is more transparent to the end user.
>>
>> All of this work was tested by passing traffic, including via a dpdk bridge.
>>
>> I'm hoping that this can be backported to the 2.8 branch (since it would be
>> needed to make fedora 2.8 make sense), but if not, we can always do the 
>> manual
>> backport
>>
> I already pushed your patches to master branch. However, before
> back-porting them to 2.8 I think more testing is required. For
> example:

Agreed.  I addressed your concerns, and also found a really
embarrassingly stupid mistake.

I plan on continuing to test it anyway.  I'll be making some beer this
weekend so I should have some spare cycles to kick stuff off.

Thanks for all your help, Ansis!

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

[ovs-dev] [PATCH v2 2/3] makefile: hook up dpdkstrip preprocessor

[Aaron Conole]

When building the openvswitch-custom.te file, it is important to have the
ability to filter out dpdk blocks depending on whether the system has been
configured with dpdk or not.  This allows using all the standard .in file
blocks, as well as the dpdkstrip blocks, when constructing the selinux
policy file.

Additionally, this means any .in files which might want to change based on
configuration to exclude blocks based on dpdk can do so.

Acked-by: Flavio Leitner <f...@sysclose.org>
Signed-off-by: Aaron Conole <acon...@redhat.com>
Tested-by: Jean Hsiao <jhs...@redhat.com>
---
 Makefile.am | 4 
 1 file changed, 4 insertions(+)

diff --git a/Makefile.am b/Makefile.am
index 035afe6..31d6331 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -35,6 +35,9 @@ AM_CFLAGS += $(OVS_CFLAGS)
 
 if DPDK_NETDEV
 AM_CFLAGS += -D_FILE_OFFSET_BITS=64
+DPDKSTRIP_FLAGS = --dpdk
+else
+DPDKSTRIP_FLAGS = --nodpdk
 endif
 
 if NDEBUG
@@ -141,6 +144,7 @@ ro_shell = printf '\043 Generated automatically -- do not 
modify!-*- buffer-
 SUFFIXES += .in
 .in:
$(AM_V_GEN)$(PERL) $(srcdir)/build-aux/soexpand.pl -I$(srcdir) < $< | \
+ $(PERL) $(srcdir)/build-aux/dpdkstrip.pl $(DPDKSTRIP_FLAGS) | \
  sed \
-e 's,[@]PKIDIR[@],$(PKIDIR),g' \
-e 's,[@]LOGDIR[@],$(LOGDIR),g' \
-- 
2.9.4

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

[ovs-dev] [PATCH v2 1/3] rhel: make the selinux policy intermediate

[Aaron Conole]

This will be used by an upcoming commit to have @begin_ and @end_ dpdk
blocks to keep dpdk specific policy decisions only active when dpdk is
used.

Acked-by: Flavio Leitner <f...@sysclose.org>
Signed-off-by: Aaron Conole <acon...@redhat.com>
Tested-by: Jean Hsiao <jhs...@redhat.com>
---
 rhel/openvswitch-fedora.spec.in | 1 +
 selinux/automake.mk | 2 +-
 selinux/{openvswitch-custom.te => openvswitch-custom.te.in} | 0
 3 files changed, 2 insertions(+), 1 deletion(-)
 rename selinux/{openvswitch-custom.te => openvswitch-custom.te.in} (100%)

diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
index 59e8ff8..dd79fa9 100644
--- a/rhel/openvswitch-fedora.spec.in
+++ b/rhel/openvswitch-fedora.spec.in
@@ -239,6 +239,7 @@ Docker network plugins for OVN.
> rhel/usr_lib_systemd_system_ovs-vswitchd.service
 
 make %{?_smp_mflags}
+make selinux/openvswitch-custom.te
 cd selinux
 make -f %{_datadir}/selinux/devel/Makefile
 
diff --git a/selinux/automake.mk b/selinux/automake.mk
index 1088f36..e8871aa 100644
--- a/selinux/automake.mk
+++ b/selinux/automake.mk
@@ -6,4 +6,4 @@
 # without warranty of any kind.
 
 EXTRA_DIST += \
-selinux/openvswitch-custom.te
+selinux/openvswitch-custom.te.in
diff --git a/selinux/openvswitch-custom.te b/selinux/openvswitch-custom.te.in
similarity index 100%
rename from selinux/openvswitch-custom.te
rename to selinux/openvswitch-custom.te.in
-- 
2.9.4

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

[ovs-dev] [PATCH v2 0/3] updated selinux policy for Open vSwitch

[Aaron Conole]

This series brings about a policy update to openvswitch allowing it to
run on a RHEL / Fedora system, even as a non-root user, with selinux set
to Enforcing.

The first two patches make some changes to the way the selinux policy is
built to have a macro-like effect, allowing the dpdk policy to be enabled
or disabled based on the build.  This is chosen instead of using an selinux
boolean, because it is more transparent to the end user.

All of this work was tested by passing traffic, including via a dpdk bridge.

I'm hoping that this can be backported to the 2.8 branch (since it would be
needed to make fedora 2.8 make sense), but if not, we can always do the manual
backport

Original Series:
https://mail.openvswitch.org/pipermail/ovs-dev/2017-August/337513.html

v1->v2:
* updated after PVP testing.  There are still permissions needed to be added
  to libvirt / qemu, but that is outside the scope of Open vSwitch project.
* Folded in Flavio Leitner's ACK

Aaron Conole (3):
  rhel: make the selinux policy intermediate
  makefile: hook up dpdkstrip preprocessor
  selinux: update policy to reflect non-root and dpdk support

 Makefile.am  |  4 +++
 rhel/openvswitch-fedora.spec.in  |  1 +
 selinux/automake.mk  |  2 +-
 selinux/openvswitch-custom.te| 16 
 selinux/openvswitch-custom.te.in | 54 
 5 files changed, 60 insertions(+), 17 deletions(-)
 delete mode 100644 selinux/openvswitch-custom.te
 create mode 100644 selinux/openvswitch-custom.te.in

-- 
2.9.4

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

Re: [ovs-dev] [PATCH v2 3/3] selinux: update policy to reflect non-root and dpdk support

[Aaron Conole]

Hi Ansis,

Thanks for the review!

Ansis Atteka <ansisatt...@gmail.com> writes:

> On 30 August 2017 at 07:00, Aaron Conole <acon...@redhat.com> wrote:
>> The selinux policy that exists in the repository did not specify access to
>> all of the resources needed for Open vSwitch to properly function with
>> an enforcing selinux policy.  This update allows Open vSwitch to operate
>> with selinux set to Enforcing mode, even while running as a non-root user.
>>
>> Acked-by: Flavio Leitner <f...@sysclose.org>
>> Signed-off-by: Aaron Conole <acon...@redhat.com>
>> Tested-by: Jean Hsiao <jhs...@redhat.com>
>> ---
>>  selinux/openvswitch-custom.te.in | 40 
>> +++-
>>  1 file changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/selinux/openvswitch-custom.te.in 
>> b/selinux/openvswitch-custom.te.in
>> index 47ddb56..66cb678 100644
>> --- a/selinux/openvswitch-custom.te.in
>> +++ b/selinux/openvswitch-custom.te.in
>> @@ -2,15 +2,53 @@ module openvswitch-custom 1.0.1;
>>
>>  require {
>>  type openvswitch_t;
>> +type openvswitch_rw_t;
>>  type openvswitch_tmp_t;
>> +type openvswitch_var_run_t;
>> +
>>  type ifconfig_exec_t;
>>  type hostname_exec_t;
>> +type tun_tap_device_t;
>> +
>> +@begin_dpdk@
>> +type hugetlbfs_t;
>> +type kernel_t;
>> +type svirt_image_t;
>> +type vfio_device_t;
>> +@end_dpdk@
>> +
>> +class capability { dac_override audit_write };
>> +class dir { write remove_name add_name lock read };
>> +class file { write getattr read open execute execute_no_trans 
>> create unlink };
>> +class netlink_audit_socket { create nlmsg_relay audit_write read 
>> write };
>>  class netlink_socket { setopt getopt create connect getattr write 
>> read };
>> -class file { write getattr read open execute execute_no_trans };
>> +class unix_stream_socket { write getattr read connectto connect 
>> setopt getopt sendto accept bind recvfrom acceptfrom };
>> +
>> +@begin_dpdk@
>> +class chr_file { write getattr read open ioctl };
>> +class tun_socket { relabelfrom relabelto create };
>> +@end_dpdk@
>>  }
>>
>>  #= openvswitch_t ==
>> +allow openvswitch_t self:capability { dac_override audit_write };
>> +allow openvswitch_t self:netlink_audit_socket { create nlmsg_relay 
>> audit_write read write };
>>  allow openvswitch_t self:netlink_socket { setopt getopt create connect 
>> getattr write read };
>> +
>>  allow openvswitch_t hostname_exec_t:file { read getattr open execute 
>> execute_no_trans };
>>  allow openvswitch_t ifconfig_exec_t:file { read getattr open execute 
>> execute_no_trans };
>> +
>> +allow openvswitch_t openvswitch_rw_t:dir { write remove_name add_name lock 
>> read };
>> +allow openvswitch_t openvswitch_rw_t:file { write getattr read open execute 
>> execute_no_trans create unlink };
>>  allow openvswitch_t openvswitch_tmp_t:file { execute execute_no_trans };
>> +allow openvswitch_t openvswitch_tmp_t:unix_stream_socket { write getattr 
>> read connectto connect setopt getopt sendto accept bind recvfrom acceptfrom 
>> };
>> +
>> +@begin_dpdk@
>> +allow openvswitch_t hugetlbfs_t:dir { write remove_name add_name lock read 
>> };
>> +allow openvswitch_t hugetlbfs_t:file { create unlink };
>> +allow openvswitch_t kernel_t:unix_stream_socket { write getattr read 
>> connectto connect setopt getopt sendto accept bind recvfrom acceptfrom };
>> +allow openvswitch_t self:tun_socket { relabelfrom relabelto create };
>
> Can you give an example why above rule is required? I believe it
> allows processes running under openvswitch_t type to change labels? I
> believe by having such rule the "Mandatory" access control becomes
> kinda like "Discretionary" in sense that process has now some
> discretion to alter SELinux policy.

So, as you point out it allows openvswitch_t to relabel from / to on
tun_socket devices.

Inline are the related AVC message records I have.

- >8 
type=AVC msg=audit(1503426353.300:21402): avc:  denied  { relabelfrom } for  
pid=128134  comm="ovs-vswitchd" scontext=system_u:system_r:openvswitch_t:s0 
tcontext=system_u:system_r:openvswitch_t:s0 tclass=tun_socket
type=AVC msg=audit(1503426353.300:21402): avc:  denied  { relabelto } for  
pid=

Re: [ovs-dev] [PATCH v2 3/3] selinux: update policy to reflect non-root and dpdk support

[Aaron Conole]

Ansis Atteka <ansisatt...@gmail.com> writes:

> On 31 August 2017 at 11:58, Aaron Conole <acon...@redhat.com> wrote:
>> Hi Ansis,
>>
>> Thanks for the review!
>>
>> Ansis Atteka <ansisatt...@gmail.com> writes:
>>
>>> On 30 August 2017 at 07:00, Aaron Conole <acon...@redhat.com> wrote:
>>>> The selinux policy that exists in the repository did not specify access to
>>>> all of the resources needed for Open vSwitch to properly function with
>>>> an enforcing selinux policy.  This update allows Open vSwitch to operate
>>>> with selinux set to Enforcing mode, even while running as a non-root user.
>>>>
>>>> Acked-by: Flavio Leitner <f...@sysclose.org>
>>>> Signed-off-by: Aaron Conole <acon...@redhat.com>
>>>> Tested-by: Jean Hsiao <jhs...@redhat.com>
>>>> ---
>>>>  selinux/openvswitch-custom.te.in | 40 
>>>> +++-
>>>>  1 file changed, 39 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/selinux/openvswitch-custom.te.in 
>>>> b/selinux/openvswitch-custom.te.in
>>>> index 47ddb56..66cb678 100644
>>>> --- a/selinux/openvswitch-custom.te.in
>>>> +++ b/selinux/openvswitch-custom.te.in
>>>> @@ -2,15 +2,53 @@ module openvswitch-custom 1.0.1;
>>>>
>>>>  require {
>>>>  type openvswitch_t;
>>>> +type openvswitch_rw_t;
>>>>  type openvswitch_tmp_t;
>>>> +type openvswitch_var_run_t;
>>>> +
>>>>  type ifconfig_exec_t;
>>>>  type hostname_exec_t;
>>>> +type tun_tap_device_t;
>>>> +
>>>> +@begin_dpdk@
>>>> +type hugetlbfs_t;
>>>> +type kernel_t;
>>>> +type svirt_image_t;
>>>> +type vfio_device_t;
>>>> +@end_dpdk@
>>>> +
>>>> +class capability { dac_override audit_write };
>>>> +class dir { write remove_name add_name lock read };
>>>> +class file { write getattr read open execute execute_no_trans 
>>>> create unlink };
>>>> +class netlink_audit_socket { create nlmsg_relay audit_write read 
>>>> write };
>>>>  class netlink_socket { setopt getopt create connect getattr write 
>>>> read };
>>>> -class file { write getattr read open execute execute_no_trans };
>>>> +class unix_stream_socket { write getattr read connectto connect 
>>>> setopt getopt sendto accept bind recvfrom acceptfrom };
>>>> +
>>>> +@begin_dpdk@
>>>> +class chr_file { write getattr read open ioctl };
>>>> +class tun_socket { relabelfrom relabelto create };
>>>> +@end_dpdk@
>>>>  }
>>>>
>>>>  #= openvswitch_t ==
>>>> +allow openvswitch_t self:capability { dac_override audit_write };
>>>> +allow openvswitch_t self:netlink_audit_socket { create nlmsg_relay 
>>>> audit_write read write };
>>>>  allow openvswitch_t self:netlink_socket { setopt getopt create connect 
>>>> getattr write read };
>>>> +
>>>>  allow openvswitch_t hostname_exec_t:file { read getattr open execute 
>>>> execute_no_trans };
>>>>  allow openvswitch_t ifconfig_exec_t:file { read getattr open execute 
>>>> execute_no_trans };
>>>> +
>>>> +allow openvswitch_t openvswitch_rw_t:dir { write remove_name add_name 
>>>> lock read };
>>>> +allow openvswitch_t openvswitch_rw_t:file { write getattr read open 
>>>> execute execute_no_trans create unlink };
>>>>  allow openvswitch_t openvswitch_tmp_t:file { execute execute_no_trans };
>>>> +allow openvswitch_t openvswitch_tmp_t:unix_stream_socket { write getattr 
>>>> read connectto connect setopt getopt sendto accept bind recvfrom 
>>>> acceptfrom };
>>>> +
>>>> +@begin_dpdk@
>>>> +allow openvswitch_t hugetlbfs_t:dir { write remove_name add_name lock 
>>>> read };
>>>> +allow openvswitch_t hugetlbfs_t:file { create unlink };
>>>> +allow openvswitch_t kernel_t:unix_stream_socket { write getattr read 
>>>> connectto connect setopt getopt sendto accept bind recvfrom acceptfrom };
>>>> +allow openvswitch_t self:tun_socket { relabelfrom relabelto create };
>>>

[ovs-dev] [PATCH v3 0/3] updated selinux policy for Open vSwitch

[Aaron Conole]

This series brings about a policy update to openvswitch allowing it to
run on a RHEL / Fedora system, even as a non-root user, with selinux set
to Enforcing.

The first two patches make some changes to the way the selinux policy is
built to have a macro-like effect, allowing the dpdk policy to be enabled
or disabled based on the build.  This is chosen instead of using an selinux
boolean, because it is more transparent to the end user.

All of this work was tested by passing traffic, including via a dpdk bridge.

I'm hoping that this can be backported to the 2.8 branch (since it would be
needed to make fedora 2.8 make sense), but if not, we can always do the manual
backport

Original Series:
https://mail.openvswitch.org/pipermail/ovs-dev/2017-August/337513.html

v2->v3:
* move tun_tap_device_t permissions to be more general purpose

v1->v2:
* updated after PVP testing.  There are still permissions needed to be added
  to libvirt / qemu, but that is outside the scope of Open vSwitch project.
* Folded in Flavio Leitner's ACK

Aaron Conole (3):
  rhel: make the selinux policy intermediate
  makefile: hook up dpdkstrip preprocessor
  selinux: update policy to reflect non-root and dpdk support

 Makefile.am  |  4 +++
 rhel/openvswitch-fedora.spec.in  |  1 +
 selinux/automake.mk  |  2 +-
 selinux/openvswitch-custom.te| 16 
 selinux/openvswitch-custom.te.in | 54 
 5 files changed, 60 insertions(+), 17 deletions(-)
 delete mode 100644 selinux/openvswitch-custom.te
 create mode 100644 selinux/openvswitch-custom.te.in

-- 
2.9.4

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

[ovs-dev] [PATCH v3 2/3] makefile: hook up dpdkstrip preprocessor

[Aaron Conole]

When building the openvswitch-custom.te file, it is important to have the
ability to filter out dpdk blocks depending on whether the system has been
configured with dpdk or not.  This allows using all the standard .in file
blocks, as well as the dpdkstrip blocks, when constructing the selinux
policy file.

Additionally, this means any .in files which might want to change based on
configuration to exclude blocks based on dpdk can do so.

Acked-by: Flavio Leitner <f...@sysclose.org>
Signed-off-by: Aaron Conole <acon...@redhat.com>
Tested-by: Jean Hsiao <jhs...@redhat.com>
---
 Makefile.am | 4 
 1 file changed, 4 insertions(+)

diff --git a/Makefile.am b/Makefile.am
index 035afe6..31d6331 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -35,6 +35,9 @@ AM_CFLAGS += $(OVS_CFLAGS)
 
 if DPDK_NETDEV
 AM_CFLAGS += -D_FILE_OFFSET_BITS=64
+DPDKSTRIP_FLAGS = --dpdk
+else
+DPDKSTRIP_FLAGS = --nodpdk
 endif
 
 if NDEBUG
@@ -141,6 +144,7 @@ ro_shell = printf '\043 Generated automatically -- do not 
modify!-*- buffer-
 SUFFIXES += .in
 .in:
$(AM_V_GEN)$(PERL) $(srcdir)/build-aux/soexpand.pl -I$(srcdir) < $< | \
+ $(PERL) $(srcdir)/build-aux/dpdkstrip.pl $(DPDKSTRIP_FLAGS) | \
  sed \
-e 's,[@]PKIDIR[@],$(PKIDIR),g' \
-e 's,[@]LOGDIR[@],$(LOGDIR),g' \
-- 
2.9.4

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

[ovs-dev] [PATCH v3 1/3] rhel: make the selinux policy intermediate

[Aaron Conole]

This will be used by an upcoming commit to have @begin_ and @end_ dpdk
blocks to keep dpdk specific policy decisions only active when dpdk is
used.

Acked-by: Flavio Leitner <f...@sysclose.org>
Signed-off-by: Aaron Conole <acon...@redhat.com>
Tested-by: Jean Hsiao <jhs...@redhat.com>
---
 rhel/openvswitch-fedora.spec.in | 1 +
 selinux/automake.mk | 2 +-
 selinux/{openvswitch-custom.te => openvswitch-custom.te.in} | 0
 3 files changed, 2 insertions(+), 1 deletion(-)
 rename selinux/{openvswitch-custom.te => openvswitch-custom.te.in} (100%)

diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
index 59e8ff8..dd79fa9 100644
--- a/rhel/openvswitch-fedora.spec.in
+++ b/rhel/openvswitch-fedora.spec.in
@@ -239,6 +239,7 @@ Docker network plugins for OVN.
> rhel/usr_lib_systemd_system_ovs-vswitchd.service
 
 make %{?_smp_mflags}
+make selinux/openvswitch-custom.te
 cd selinux
 make -f %{_datadir}/selinux/devel/Makefile
 
diff --git a/selinux/automake.mk b/selinux/automake.mk
index 1088f36..e8871aa 100644
--- a/selinux/automake.mk
+++ b/selinux/automake.mk
@@ -6,4 +6,4 @@
 # without warranty of any kind.
 
 EXTRA_DIST += \
-selinux/openvswitch-custom.te
+selinux/openvswitch-custom.te.in
diff --git a/selinux/openvswitch-custom.te b/selinux/openvswitch-custom.te.in
similarity index 100%
rename from selinux/openvswitch-custom.te
rename to selinux/openvswitch-custom.te.in
-- 
2.9.4

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

Re: [ovs-dev] [PATCH v2 3/3] selinux: update policy to reflect non-root and dpdk support

[Aaron Conole]

Ansis Atteka <ansisatt...@gmail.com> writes:

> On 31 August 2017 at 14:57, Aaron Conole <acon...@redhat.com> wrote:
>> Ansis Atteka <ansisatt...@gmail.com> writes:
>>
>>> On 31 August 2017 at 11:58, Aaron Conole <acon...@redhat.com> wrote:
>>>> Hi Ansis,
>>>>
>>>> Thanks for the review!
>>>>
>>>> Ansis Atteka <ansisatt...@gmail.com> writes:
>>>>
>>>>> On 30 August 2017 at 07:00, Aaron Conole <acon...@redhat.com> wrote:
>>>>>> The selinux policy that exists in the repository did not specify access 
>>>>>> to
>>>>>> all of the resources needed for Open vSwitch to properly function with
>>>>>> an enforcing selinux policy.  This update allows Open vSwitch to operate
>>>>>> with selinux set to Enforcing mode, even while running as a non-root 
>>>>>> user.
>>>>>>
>>>>>> Acked-by: Flavio Leitner <f...@sysclose.org>
>>>>>> Signed-off-by: Aaron Conole <acon...@redhat.com>
>>>>>> Tested-by: Jean Hsiao <jhs...@redhat.com>
>>>>>> ---
>>>>>>  selinux/openvswitch-custom.te.in | 40 
>>>>>> +++-
>>>>>>  1 file changed, 39 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/selinux/openvswitch-custom.te.in 
>>>>>> b/selinux/openvswitch-custom.te.in
>>>>>> index 47ddb56..66cb678 100644
>>>>>> --- a/selinux/openvswitch-custom.te.in
>>>>>> +++ b/selinux/openvswitch-custom.te.in
>>>>>> @@ -2,15 +2,53 @@ module openvswitch-custom 1.0.1;
>>>>>>
>>>>>>  require {
>>>>>>  type openvswitch_t;
>>>>>> +type openvswitch_rw_t;
>>>>>>  type openvswitch_tmp_t;
>>>>>> +type openvswitch_var_run_t;
>>>>>> +
>>>>>>  type ifconfig_exec_t;
>>>>>>  type hostname_exec_t;
>>>>>> +type tun_tap_device_t;
>>>>>> +
>>>>>> +@begin_dpdk@
>>>>>> +type hugetlbfs_t;
>>>>>> +type kernel_t;
>>>>>> +type svirt_image_t;
>>>>>> +type vfio_device_t;
>>>>>> +@end_dpdk@
>>>>>> +
>>>>>> +class capability { dac_override audit_write };
>>>>>> +class dir { write remove_name add_name lock read };
>>>>>> +class file { write getattr read open execute execute_no_trans 
>>>>>> create unlink };
>>>>>> +class netlink_audit_socket { create nlmsg_relay audit_write 
>>>>>> read write };
>>>>>>  class netlink_socket { setopt getopt create connect getattr 
>>>>>> write read };
>>>>>> -class file { write getattr read open execute execute_no_trans };
>>>>>> +class unix_stream_socket { write getattr read connectto connect 
>>>>>> setopt getopt sendto accept bind recvfrom acceptfrom };
>>>>>> +
>>>>>> +@begin_dpdk@
>>>>>> +class chr_file { write getattr read open ioctl };
>>>>>> +class tun_socket { relabelfrom relabelto create };
>>>>>> +@end_dpdk@
>>>>>>  }
>>>>>>
>>>>>>  #= openvswitch_t ==
>>>>>> +allow openvswitch_t self:capability { dac_override audit_write };
>>>>>> +allow openvswitch_t self:netlink_audit_socket { create nlmsg_relay 
>>>>>> audit_write read write };
>>>>>>  allow openvswitch_t self:netlink_socket { setopt getopt create connect 
>>>>>> getattr write read };
>>>>>> +
>>>>>>  allow openvswitch_t hostname_exec_t:file { read getattr open execute 
>>>>>> execute_no_trans };
>>>>>>  allow openvswitch_t ifconfig_exec_t:file { read getattr open execute 
>>>>>> execute_no_trans };
>>>>>> +
>>>>>> +allow openvswitch_t openvswitch_rw_t:dir { write remove_name add_name 
>>>>>> lock read };
>>>>>> +allow openvswitch_t openvswitch_rw_t:file { write getattr read open 
>>>>>>

[ovs-dev] [PATCH v3 3/3] selinux: update policy to reflect non-root and dpdk support

[Aaron Conole]

The selinux policy that exists in the repository did not specify access to
all of the resources needed for Open vSwitch to properly function with
an enforcing selinux policy.  This update allows Open vSwitch to operate
with selinux set to Enforcing mode, even while running as a non-root user.

Acked-by: Flavio Leitner <f...@sysclose.org>
Signed-off-by: Aaron Conole <acon...@redhat.com>
Tested-by: Jean Hsiao <jhs...@redhat.com>
---
 selinux/openvswitch-custom.te.in | 40 +++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/selinux/openvswitch-custom.te.in b/selinux/openvswitch-custom.te.in
index 47ddb56..66cb678 100644
--- a/selinux/openvswitch-custom.te.in
+++ b/selinux/openvswitch-custom.te.in
@@ -2,15 +2,53 @@ module openvswitch-custom 1.0.1;
 
 require {
 type openvswitch_t;
+type openvswitch_rw_t;
 type openvswitch_tmp_t;
+type openvswitch_var_run_t;
+
 type ifconfig_exec_t;
 type hostname_exec_t;
+type tun_tap_device_t;
+
+@begin_dpdk@
+type hugetlbfs_t;
+type kernel_t;
+type svirt_image_t;
+type vfio_device_t;
+@end_dpdk@
+
+class capability { dac_override audit_write };
+class dir { write remove_name add_name lock read };
+class file { write getattr read open execute execute_no_trans create 
unlink };
+class netlink_audit_socket { create nlmsg_relay audit_write read write 
};
 class netlink_socket { setopt getopt create connect getattr write read 
};
-class file { write getattr read open execute execute_no_trans };
+class unix_stream_socket { write getattr read connectto connect setopt 
getopt sendto accept bind recvfrom acceptfrom };
+
+@begin_dpdk@
+class chr_file { write getattr read open ioctl };
+class tun_socket { relabelfrom relabelto create };
+@end_dpdk@
 }
 
 #= openvswitch_t ==
+allow openvswitch_t self:capability { dac_override audit_write };
+allow openvswitch_t self:netlink_audit_socket { create nlmsg_relay audit_write 
read write };
 allow openvswitch_t self:netlink_socket { setopt getopt create connect getattr 
write read };
+
 allow openvswitch_t hostname_exec_t:file { read getattr open execute 
execute_no_trans };
 allow openvswitch_t ifconfig_exec_t:file { read getattr open execute 
execute_no_trans };
+
+allow openvswitch_t openvswitch_rw_t:dir { write remove_name add_name lock 
read };
+allow openvswitch_t openvswitch_rw_t:file { write getattr read open execute 
execute_no_trans create unlink };
 allow openvswitch_t openvswitch_tmp_t:file { execute execute_no_trans };
+allow openvswitch_t openvswitch_tmp_t:unix_stream_socket { write getattr read 
connectto connect setopt getopt sendto accept bind recvfrom acceptfrom };
+allow openvswitch_t tun_tap_device_t:chr_file { read write getattr open ioctl 
};
+
+@begin_dpdk@
+allow openvswitch_t hugetlbfs_t:dir { write remove_name add_name lock read };
+allow openvswitch_t hugetlbfs_t:file { create unlink };
+allow openvswitch_t kernel_t:unix_stream_socket { write getattr read connectto 
connect setopt getopt sendto accept bind recvfrom acceptfrom };
+allow openvswitch_t self:tun_socket { relabelfrom relabelto create };
+allow openvswitch_t svirt_image_t:file { getattr read write };
+allow openvswitch_t vfio_device_t:chr_file { read write open ioctl getattr };
+@end_dpdk@
-- 
2.9.4

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

Re: [ovs-dev] [PATCH] RPM: Improve doc to use builddep tool.

[Aaron Conole]

Flavio Leitner  writes:

> Instead of listing all the dependencies, use the RPM group
> 'Development Tools' and the builddep tool to find specific
> ones.
>
> Signed-off-by: Flavio Leitner 
> ---
>  Documentation/intro/install/fedora.rst | 54 
> +++---
>  Documentation/intro/install/rhel.rst   | 34 +++--
>  2 files changed, 62 insertions(+), 26 deletions(-)
>
> diff --git a/Documentation/intro/install/fedora.rst
> b/Documentation/intro/install/fedora.rst
> index 3119bd9aa..70cebfb16 100644
> --- a/Documentation/intro/install/fedora.rst
> +++ b/Documentation/intro/install/fedora.rst
> @@ -36,23 +36,43 @@ RHEL 7.x and its derivatives, including CentOS 7.x
> and Scientific Linux 7.x.
>  Build Requirements
>  --
>  
> -To build packages for a Fedora Linux host, you will need the packages 
> described
> -in the :doc:`general`. Specific packages (by package name) include:
> -
> -- rpm-build
> -- autoconf automake libtool
> -- systemd-units openssl openssl-devel
> -- python2-devel python3-devel
> -- python2 python2-twisted python2-zope-interface python2-six python2-sphinx
> -- desktop-file-utils
> -- groff graphviz
> -- procps-ng
> -- checkpolicy selinux-policy-devel
> -
> -And (optionally):
> -
> -- libcap-ng libcap-ng-devel
> -- dpdk-devel
> +You will need to install all required packages to build the RPMs.
> +Newer distributions use ``dnf`` but if it's not available, then use
> +``yum`` instructions.
> +
> +The command below will install RPM tools and generic build dependencies.
> +And (optionally) include these packages: libcap-ng libcap-ng-devel 
> dpdk-devel.
> +
> +DNF:
> +::
> +
> +$ dnf install @'Development Tools' rpm-build dnf-plugins-core
> +
> +YUM:
> +::
> +
> +$ yum install @'Development Tools' rpm-build yum-utils
> +
> +Then it is necessary to install Open vSwitch specific build dependencies.
> +The dependencies are listed in the SPEC file, but first it is necessary
> +to replace the VERSION tag to be a valid SPEC.
> +
> +The command below will create a temporary SPEC file:
> +::
> +$ sed -e 's/@VERSION@/0.0.1/' rhel/openvswitch-fedora.spec.in \
> +  > /tmp/ovs.spec
> +
> +And to install specific dependencies, use the corresponding tool below.

Is there any reason we can't `make rhel/openvswitch-fedora.spec` for
this?  I think ./boot && ./configure would work with Development Tools
installed, and then the only thing left is let the make system create
just that file?

> +DNF:
> +::
> +$ dnf builddep /tmp/ovs.spec
> +
> +YUM:
> +::
> +$ yum-builddep /tmp/ovs.spec
> +
> +Once that is completed, remove the file ``/tmp/ovs.spec``.
>  
>  Bootstraping
>  
> diff --git a/Documentation/intro/install/rhel.rst
> b/Documentation/intro/install/rhel.rst
> index 0ef6f55a3..184089e89 100644
> --- a/Documentation/intro/install/rhel.rst
> +++ b/Documentation/intro/install/rhel.rst
> @@ -70,17 +70,33 @@ directory is ``/usr/src/redhat/SOURCES``. On RHEL
> 6, the default ``_topdir`` is
>  Build Requirements
>  --
>  
> -To compile the RPMs, you will need to install the packages described in the
> -:doc:`general` along with some additional packages. These can be installed 
> with
> -the below command::
> +You will need to install all required packages to build the RPMs.
> +The command below will install RPM tools and generic build dependencies:
> +::
> +$ yum install @'Development Tools' rpm-build yum-utils
>  
> -$ yum install gcc make python-devel openssl-devel kernel-devel graphviz \
> -kernel-debug-devel autoconf automake rpm-build redhat-rpm-config \
> -libtool checkpolicy selinux-policy-devel python-sphinx
> +Then it is necessary to install Open vSwitch specific build dependencies.
> +The dependencies are listed in the SPEC file, but first it is necessary
> +to replace the VERSION tag to be a valid SPEC.
>  
> -.. note::
> -  If python-sphinx package is not available in your version of RHEL, you can
> -  install it via pip with 'pip install sphinx'.
> +The command below will create a temporary SPEC file:
> +::
> +$ sed -e 's/@VERSION@/0.0.1/' rhel/openvswitch.spec.in > /tmp/ovs.spec
> +
> +And to install specific dependencies, use yum-builddep tool:
> +::
> +$ yum-builddep /tmp/ovs.spec
> +
> +Once that is completed, remove the file ``/tmp/ovs.spec``.
> +
> +If python-sphinx package is not available in your version of RHEL, you can
> +install it via pip with 'pip install sphinx'.
> +
> +Open vSwitch requires python 2.7 or newer which is not available in older
> +distributions. In the case of RHEL 6.x and its derivatives, one option is
> +to install python34 and python34-six from `EPEL`_.
> +
> +.. _EPEL: https://fedoraproject.org/wiki/EPEL
>  
>  .. _rhel-bootstrapping:
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [dpdk-dev] [PATCH RFC] netdev-dpdk: Fix device obtain mac address when received first packet in vhost type

[Aaron Conole]

"Tan, Jianfeng" <jianfeng@intel.com> writes:

> On 11/27/2017 10:27 PM, Yuanhan Liu wrote:
>> On Fri, Nov 24, 2017 at 05:59:09PM +0800, Chen Hailin wrote:
>>> Hi Aaron Conole && Jianfeng,
>>>
>>> The stp could not work in ovs-dpdk vhostuser.
>>> Because the attached vhost device doesn't have MAC address.
>>>
>>> Now we have two ways to solve this problem.
>>> 1. The vhost learns MAC address from packet like as my first patch.
>> I do agree with Aaron this is not the right way.
>
> I do think it should be the vswitch's responsibility to learn mac of
> vhost port.
>
> Except that it's the only feasible way without modifying the spec
> (yuanhan already makes it very clear below), we can treat the vswitch
> as a phsical switch, VM as a physical server, virtio/vhost port as a
> back-to-back connected NICs, the only way of the physical switch to
> know the mac of the NIC on the other side is ARP learning.
>
> Might I ask why you don't think it's a right way?

As a quick example, I think a malicious guest in a multi-tenant
environment could send traffic out to manipulate this feature into
learning an incorrect mac address.

To get this right requires doing deep packet inspection, and making sure
to only learn based on certain l2 traffic.

> Thanks,
> Jianfeng
>
>>
>>> 2. The virtio notifies MAC address actively to vhost user .
>> Unfortunately, AFAIK, there is no way to achieve that so far. we could
>> either let virtio/QEMU to expose the CQ to vhost or add a new VHOST_USER
>> message to carry the mac address. While vhost-user is a generic interface
>> adding a virtio-net specific message also doesn't seem quite right.
>> Exposing CQ is probably the best we can do.
>>
>> Anyway, both need spec change.
>>
>>  --yliu
>>> In my opinions,  if we treat it as a device,  we should allocate
>>> MAC address for the device when the VM started.
>>>
>>> Which one do you think better?
>>>
>>>
>>>
>>> Best Regards,
>>> Chen Hailin
>>> che...@arraynetworks.com.cn
>>>   From: Aaron Conole
>>> Date: 2017-11-18 10:00
>>> To: Hailin Chen
>>> CC: ovs-dev@openvswitch.org; Maxime Coquelin; cl...@arraynetworks.com.cn
>>> Subject: Re: [ovs-dev] [PATCH RFC] netdev-dpdk: Fix device obtain
>>> mac address when received first packet in vhost type
>>> Hi Hailin,
>>>   Hailin Chen <che...@arraynetworks.com.cn> writes:
>>>   
>>>> The stp could not work on netdev-dpdk if network is loop.
>>>> Because the stp protocol negotiates designate port by sending
>>>> BPDU packets which contains MAC address.
>>>> However the device doesn't have MAC address in vhostuser type.
>>>> Thus, function send_bpdu_cb would not send BPDU packets.
>>>>
>>>> This patch will set the MAC for device when received first packet.
>>>>
>>>> Signed-off-by: Hailin Chen <che...@arraynetworks.com.cn>
>>>> ---
>>>   Thanks for the patch.
>>>   In general, I don't think this is the right approach to deal with
>>> this
>>> type of issue.  I believe the problem statement is that OvS bridge is
>>> unaware of the guest MAC address - did I get it right?  In that case, I
>>> would think that a better way to solve this would be to have virtio tell
>>> the mac address of the guest.  I don't recall right now if that's
>>> allowed in the virtio spec, but I do remember some kind of negotiation
>>> features.
>>>   I've CC'd Maxime, who is one of the maintainers of the virtio
>>> code from
>>> DPDK side.  Perhaps there is an alternate way to solve this.
>>> ___
>>> dev mailing list
>>> d...@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [dpdk-dev] [PATCH RFC] netdev-dpdk: Fix device obtain mac address when received first packet in vhost type

[Aaron Conole]

Yuanhan Liu <y...@fridaylinux.org> writes:

> On Fri, Nov 24, 2017 at 05:59:09PM +0800, Chen Hailin wrote:
>> Hi Aaron Conole && Jianfeng,
>> 
>> The stp could not work in ovs-dpdk vhostuser.
>> Because the attached vhost device doesn't have MAC address.
>> 
>> Now we have two ways to solve this problem.
>> 1. The vhost learns MAC address from packet like as my first patch.
>
> I do agree with Aaron this is not the right way.
>
>> 2. The virtio notifies MAC address actively to vhost user .
>
> Unfortunately, AFAIK, there is no way to achieve that so far. we could
> either let virtio/QEMU to expose the CQ to vhost or add a new VHOST_USER
> message to carry the mac address. While vhost-user is a generic interface
> adding a virtio-net specific message also doesn't seem quite right.
> Exposing CQ is probably the best we can do.
>
> Anyway, both need spec change.

There are other possible ways.  libvirt knows about the mac address from
the domain xml file.  Perhaps it's possible to set the mac column in the
database to the correct value when the port is being created in ovs?
This would be an action taken by the orchestration tool.

Additionally, there's a mechanism in virtio-net to set the mac address
from the host to the guest.  Is it possible to expose that functionality
through vhost-user?

Then when the orchestration tool sets the mac, it can be propagated, and
mac_in_use can reflect the appropriate value.  I think that's a workable
solution, but I might have missed something.

>   --yliu
>> 
>> In my opinions,  if we treat it as a device,  we should allocate 
>> MAC address for the device when the VM started.
>> 
>> Which one do you think better?
>> 
>> 
>> 
>> Best Regards,
>> Chen Hailin
>> che...@arraynetworks.com.cn
>>  
>> From: Aaron Conole
>> Date: 2017-11-18 10:00
>> To: Hailin Chen
>> CC: ovs-dev@openvswitch.org; Maxime Coquelin; cl...@arraynetworks.com.cn
>> Subject: Re: [ovs-dev] [PATCH RFC] netdev-dpdk: Fix device obtain
>> mac address when received first packet in vhost type
>> Hi Hailin,
>>  
>> Hailin Chen <che...@arraynetworks.com.cn> writes:
>>  
>> > The stp could not work on netdev-dpdk if network is loop.
>> > Because the stp protocol negotiates designate port by sending
>> > BPDU packets which contains MAC address.
>> > However the device doesn't have MAC address in vhostuser type.
>> > Thus, function send_bpdu_cb would not send BPDU packets.
>> >
>> > This patch will set the MAC for device when received first packet.
>> >
>> > Signed-off-by: Hailin Chen <che...@arraynetworks.com.cn>
>> > ---
>>  
>> Thanks for the patch.
>>  
>> In general, I don't think this is the right approach to deal with this
>> type of issue.  I believe the problem statement is that OvS bridge is
>> unaware of the guest MAC address - did I get it right?  In that case, I
>> would think that a better way to solve this would be to have virtio tell
>> the mac address of the guest.  I don't recall right now if that's
>> allowed in the virtio spec, but I do remember some kind of negotiation
>> features.
>>  
>> I've CC'd Maxime, who is one of the maintainers of the virtio code from
>> DPDK side.  Perhaps there is an alternate way to solve this.
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [CONNTRACK] Discussions at OvS 2017

[Aaron Conole]

Tiago Lam <tiago...@gmail.com> writes:

> Hi all,
>
> Thanks, Darrell. That sounds reasonable to me, nothing to point there.

I agree.  We need to ensure that pollution of code doesn't occur.

> But this leads me to my next question; With regards to integrating
> with SIP (or even SCTP), is there a list of the main use cases to
> support, or something similar? I guess this is still early days, but
> for SIP for example, I'd imagine that supporting the INVITE and
> respective 200 OK and inspecting the SDP to set up the expectations
> for the RTP connections would be a good first step. But what other
> methods are of interest out there?

I'll answer from my perspective.

I'll ask our customers for more firm requirements.  I know we have
customers who are looking to adopt OvS on their hardware, and process
10s of calls per second, who are looking at what exists as far as
ovs features goes.  I think you've outlined the important messages as
far as SIP goes.

As for SCTP, that I'm less aware.  I know we have customers who use SCTP
for processing messages (and newer AAA services use DIAMETER which has
some dependency on SCTP).

Does that help understand?

> Regards,
> Tiago.
>
> On 11/25/2017 07:37 PM, Darrell Ball wrote:
>> Let me clarify some general points.
>> 
>> 1/ We will not be porting any code from the Linux kernel, whether it
>> be SIP module code or anything else.
>> 
>> 2/ Furthermore, we will not be using proprietary code algorithms and
>> other aspects from the Linux kernel.
>> 
>> 3/ Furthermore, those people working in, having worked in, or having
>> been exposed to Netfilter code need to
>> be careful about any “cross-pollination”, intentional or otherwise.
>> 
>> 4/ In addition, in general, we don’t think it is necessary to copy
>> the behavior of the kernel; we look at
>> each behavior and see if it makes sense or is necessary on its own
>> technical merits. If we don’t think it
>> necessary, overly complex, or we have something better, we can omit
>> that behavior entirely or do it differently.
>> 
>> Thanks Darrell
>> 
>> 
>> 
>> On 11/25/17, 9:47 AM, "ovs-dev-boun...@openvswitch.org on behalf of
>> Tiago Lam" <ovs-dev-boun...@openvswitch.org on behalf of
>> tiago...@gmail.com> wrote:
>> 
>> Hi Aaron (and all),
>> 
>> Thank you for your email, I found it to be informative.
>> 
>> Would you mind elaborating a bit more on point number 5 below?
>> 
>> With regards to SIP (don't know about SCTP), a module [1] seems to 
>> already
>> exist for the kernel, integrating with Netfilter / conntrack.
>> 
>> So, in terms of support for SIP in OVS' userspace conntrack (or any 
>> other new
>> protocol for that matter, but I'm most familiar with SIP), would OVS be
>> looking for a port, or a "from scratch" implementation? Or what would be 
>> the
>> preference here?
>> 
>> Thanks and regards,
>>     
>> Tiago.
>> 
>> [1]
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__people.netfilter.org_chentschel_docs_sip-2Dconntrack-2Dnat.html=DwICAg=uilaK90D4TOVoH58JNXRgQ=BVhFA09CGX7JQ5Ih-uZnsw=FfTX_mff2SgY4OCYC5YbZOO44fljLYgir7rk9fccjRQ=Pfb29ck4yGzJEN3CgHQWatvLFA-XFUuMG_UWAKoutN0=
>> 
>> On 11/20/2017 06:21 PM, Aaron Conole wrote:
>> > (NOTE: This is a resend - I fat-fingered the ovs email.  Apologies to
>> > those who got duplicates).
>> > 
>> > This email is meant to summarize some of the discussions we had at OvS
>> > conference.
>> > 
>> > The interest in the userspace conntrack is heating up.  That's a good
>> > thing, but it means that we'll probably have some growing pains as it
>> > relates to features and usages.  These are the topics and some
>> > additional information that we came up with.
>> > 
>> > 1. Making EST state match between linux kernel conntrack and userspace
>> >conntrack.  We will need to make sure that the windows datapath
>> >conntrack also matches.  The issue came down to the distinction 
>> about
>> >whether commit action should also imply that the connection is
>> >established.
>> > 
>> > 2. Disabling conntrack helpers 'on by default' in the userspace
>> >datapath.  This represents possible security issue; users will want
>> >to disable helpers (or enable helpers) at their own discretion.
>> >One pro

Re: [ovs-dev] [PATCH] redhat: Create /etc/openvswitch/* with openvswitch as user/group

[Aaron Conole]

Timothy Redaelli <tredae...@redhat.com> writes:

> Without this commit is not possible to upgrade an openvswitch release
> that includes the commit ac416a3ab2d2 (for example 2.8.0) with another release
> that includes the commit ac416a3ab2d2 (for example master or 2.8.1), because
> rpm changes the user/group of /etc/openvswitch to root/root, but ovsdb-server
> starts with the user openvswitch and so it doesn't have permissions to write 
> in
> /etc/openvswitch/conf.db.
>
> This patch tell rpm to use the openvswitch user and group for
> /etc/openvswitch and /etc/openvswitch/default.conf.
>
> Reported-by: Mark Michelson <mmich...@redhat.com>
> CC: aaron conole <acon...@redhat.com>
> Fixes: ac416a3ab2d2 ("redhat: dynamically allocate and reference ovs user")
> Signed-off-by: Timothy Redaelli <tredae...@redhat.com>
> ---

Ugh.  I guess this is only a problem if you install ovs 2.8, and then
upgrade before creating the database?

Regardless

Acked-by: Aaron Conole <acon...@redhat.com>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] redhat: Create /etc/openvswitch/* with openvswitch as user/group

[Aaron Conole]

Mark Michelson <mmich...@redhat.com> writes:

> On Wed, Nov 29, 2017 at 10:54 AM Aaron Conole <acon...@redhat.com> wrote:
>
>  Timothy Redaelli <tredae...@redhat.com> writes:
>
>  > Without this commit is not possible to upgrade an openvswitch release
>  > that includes the commit ac416a3ab2d2 (for example 2.8.0) with another 
> release
>  > that includes the commit ac416a3ab2d2 (for example master or 2.8.1), 
> because
>  > rpm changes the user/group of /etc/openvswitch to root/root, but 
> ovsdb-server
>  > starts with the user openvswitch and so it doesn't have permissions to 
> write in
>  > /etc/openvswitch/conf.db.
>  >
>  > This patch tell rpm to use the openvswitch user and group for
>  > /etc/openvswitch and /etc/openvswitch/default.conf.
>  >
>  > Reported-by: Mark Michelson <mmich...@redhat.com>
>  > CC: aaron conole <acon...@redhat.com>
>  > Fixes: ac416a3ab2d2 ("redhat: dynamically allocate and reference ovs user")
>  > Signed-off-by: Timothy Redaelli <tredae...@redhat.com>
>  > ---
>
>  Ugh.  I guess this is only a problem if you install ovs 2.8, and then
>  upgrade before creating the database?
>
>  Regardless
>
>  Acked-by: Aaron Conole <acon...@redhat.com>
>
> Nope, in my case I was installing OVS 2.8.0, starting openvswitch, 
> ovn-central, and ovn-controller
> services. I added information to the external_ids column of the open_vswitch 
> table so that the
> ovn-controller could connect to the OVN southbound database. I ensured that 
> ovn-sbctl reported the
> chassis as expected.
>
> Then I performed the upgrade. After upgrading the RPMs, /etc/openvswitch's 
> ownership had changed
> from openvswitch:openvswitch to root:root.  Attempting to restart the 
> ovs-vswitchd service at this point
> failed.

Ouch.  I thought I had confirmed the ability to upgrade again... somehow
I guess my testing wasn't sufficient.

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

Re: [ovs-dev] [patch v2 3/3] conntrack: Disable algs by default.

[Aaron Conole]

Darrell Ball  writes:

> There is a bug here in that the control connection should still be created 
> even though it is likely an unused control connection.
> The following incremental fixes it.

Good catch.  While thinking about it, I was wondering if it would be
possible to reject flows that attempt to add insane alg= helpers.  Such
a check currently exists, but only when building ofp messages (at least
as far as I can tell).  Anyway, that path lies outside the scope of this
patch but it's something to consider.

Please keep my ack if you repost with this incremental.

Thanks, Darrell!

> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index fed9f61..18016c1 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -505,7 +505,7 @@ handle_alg_ctl(struct conntrack *ct, const struct 
> conn_lookup_ctx *ctx,
> const struct conn *conn_for_expectation)
>  {
>  /* ALG control packet handling with expectation creation. */
> -if (OVS_UNLIKELY(alg_helpers[ct_alg_ctl] && conn)) {
> +if (OVS_UNLIKELY(alg_helpers[ct_alg_ctl] && conn && conn->alg)) {
>  alg_helpers[ct_alg_ctl](ct, ctx, pkt, conn_for_expectation, now,
>  CT_FTP_CTL_INTEREST, nat);
>  }
> @@ -871,15 +871,14 @@ conn_not_found(struct conntrack *ct, struct dp_packet 
> *pkt,
>  return nc;
>  }
>  
> -if (!ct_verify_helper(helper, ct_alg_ctl)) {
> -return nc;
> -}
> -
>  nc = new_conn(>buckets[bucket], pkt, >key, now);
>  ctx->conn = nc;
>  nc->rev_key = nc->key;
>  conn_key_reverse(>rev_key);
> -nc->alg = nullable_xstrdup(helper);
> +
> +if (ct_verify_helper(helper, ct_alg_ctl)) {
> +nc->alg = nullable_xstrdup(helper);
> +}
>  
>  if (alg_exp) {
>  nc->alg_related = true;
>
>
> This deserves a test case and I created one, which I will submit with this 
> patch
>
> Darrell
>
>
> On 11/22/17, 12:56 AM, "ovs-dev-boun...@openvswitch.org on behalf of Darrell 
> Ball"  wrote:
>
> Presently, alg processing is enabled by default to better exercise code.
> This is similar to kernels before 4.7 as well.  The recommended default
> behavior in the newer kernels is to only process algs if a helper is
> supplied in a conntrack rule.  The behavior is changed to match the
> later kernels.
> 
> Signed-off-by: Darrell Ball 
> ---
>  lib/conntrack.c | 35 +--
>  1 file changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 1364d05..fed9f61 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -819,6 +819,26 @@ conn_clean(struct conntrack *ct, struct conn *conn,
>  }
>  }
>  
> +static bool
> +ct_verify_helper(const char *helper, enum ct_alg_ctl_type ct_alg_ctl)
> +{
> +if (ct_alg_ctl == CT_ALG_CTL_NONE) {
> +return true;
> +} else if (helper) {
> +if ((ct_alg_ctl == CT_ALG_CTL_FTP) &&
> + !strncmp(helper, "ftp", strlen("ftp"))) {
> +return true;
> +} else if ((ct_alg_ctl == CT_ALG_CTL_TFTP) &&
> +   !strncmp(helper, "tftp", strlen("tftp"))) {
> +return true;
> +} else {
> +return false;
> +}
> +} else {
> +return false;
> +}
> +}
> +
>  /* This function is called with the bucket lock held. */
>  static struct conn *
>  conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
> @@ -826,7 +846,8 @@ conn_not_found(struct conntrack *ct, struct dp_packet 
> *pkt,
> const struct nat_action_info_t *nat_action_info,
> struct conn *conn_for_un_nat_copy,
> const char *helper,
> -   const struct alg_exp_node *alg_exp)
> +   const struct alg_exp_node *alg_exp,
> +   enum ct_alg_ctl_type ct_alg_ctl)
>  {
>  unsigned bucket = hash_to_bucket(ctx->hash);
>  struct conn *nc = NULL;
> @@ -850,14 +871,15 @@ conn_not_found(struct conntrack *ct, struct 
> dp_packet *pkt,
>  return nc;
>  }
>  
> +if (!ct_verify_helper(helper, ct_alg_ctl)) {
> +return nc;
> +}
> +
>  nc = new_conn(>buckets[bucket], pkt, >key, now);
>  ctx->conn = nc;
>  nc->rev_key = nc->key;
>  conn_key_reverse(>rev_key);
> -
> -if (helper) {
> -nc->alg = xstrdup(helper);
> -}
> +nc->alg = nullable_xstrdup(helper);
>  
>  if (alg_exp) {
>  nc->alg_related = true;
> @@ -1222,7 +1244,8 @@ 

Re: [ovs-dev] [patch v2 0/3] conntrack: Alg improvements.

[Aaron Conole]

Aaron Conole <acon...@redhat.com> writes:

> Ben Pfaff <b...@ovn.org> writes:
>
>> On Mon, Nov 27, 2017 at 06:11:42PM -0500, Aaron Conole wrote:
>>> Darrell Ball <dlu...@gmail.com> writes:
>>> 
>>> > Some refactoring of alg support is done.
>>> > Also algs are disabled by default unless an alg specifier
>>> > is supplied; this allows for enhanced security and matches
>>> > later kernels.
>>> > Another change to allow for non-standard alg conntrol
>>> > port specification.
>>> >  
>>> >
>>> > Darrell Ball (3):
>>> >   conntrack: Refactor algs.
>>> >   conntrack: Allow specified alg port numbers.
>>> >   conntrack: Disable algs by default.
>>> >
>>> >  lib/conntrack.c| 168 
>>> > ++---
>>> >  lib/conntrack.h|   8 +--
>>> >  lib/dpif-netdev.c  |   4 +-
>>> >  tests/test-conntrack.c |   6 +-
>>> >  4 files changed, 127 insertions(+), 59 deletions(-)
>>> 
>>> For the series -
>>> 
>>> Acked-by: Aaron Conole <acon...@redhat.com>
>>
>> Hi Aaron.  Thank you for reviewing--it is time consuming work and does
>> not get enough credit.  Can you describe your review process a little?
>
> Sure.  For this, the original work was at:
>   https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/341035.html
>   https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/341037.html
>
> Darrel and I had some discussions there, and I asked for this to be
> split out.  Additionally, I had noticed while reviewing that it didn't
> seem non-standard ports were covered for helpers, so asked that such a
> change be folded in (and Darrell did).
>
> For the code:
> I applied the code, and didn't see anything obviously wrong at the end
> (although I did the whole series at once, in the v2 rather than doing
> patch-at-a-time).  I could still read through the conntrack code, and
> presumably I am able to follow what is happening.
>
> Additionally, once applied, I ran `./utilities/checkpatch.py -3` to
> ensure that nothing slipped by.
>
> For the functionality:
> I ran the l7 tests via `make check-system-userspace`, and saw that the
> l7 ftp tests were passing.  This will require installation of pyftpdlib
> and tftpy to execute properly (otherwise they'll be skipped).  Those
> tests are explicitly using 'alg=', so should still pass (and they do).
>
> I didn't check anything with manual sets of flows.  I will do that now,
> though since outlining this makes me realize I never checked the
> 'negative' case (meaning assume that no alg= is given, ensure that the
> helper really isn't assigned).  I can report back on that tomorrow, if
> it makes sense.

So, I just did a simple test -

  table=0,priority=1,action=drop
  table=0,priority=10,arp,action=normal
  table=0,priority=10,icmp,action=normal
  table=0,priority=100,in_port=veth4,tcp,tp_dst=2121,action=ct(table=1)
  table=0,priority=90,in_port=veth4,tcp,action=ct(table=2)
  table=0,priority=100,in_port=veth2,tcp,action=ct(table=2)

  table=1,in_port=veth4,tcp,ct_state=+trk+new,action=ct(commit),veth2
  table=1,in_port=veth4,tcp,ct_state=+trk+est,action=veth2
  table=2,in_port=veth4,tcp,ct_state=+trk+est,action=veth2
  table=2,in_port=veth2,tcp,ct_state=+trk+est,action=veth4

with two namespaces.  Please forgive the funky ruleset - I was checking
something else out.

[root@wsfd-netdev61 ovs]# ovs-appctl dpctl/dump-conntrack netdev@ovs-netdev
tcp,orig=(src=192.168.1.2,dst=192.168.1.1,sport=59804,dport=2121),reply=(src=192.168.1.1,dst=192.168.1.2,sport=2121,dport=59804),protoinfo=(state=ESTABLISHED)

And my client process:

[root@wsfd-netdev61 ~]# ip netns exec ftp_client wget ftp://192.168.1.1:2121/ 
--no-passive-ftp
--2017-11-30 10:45:46--  ftp://192.168.1.1:2121/
   => ‘.listing’
Connecting to 192.168.1.1:2121... connected.
Logging in as anonymous ... Logged in!
==> SYST ... done.==> PWD ... done.
==> TYPE I ... done.  ==> CWD not needed.
==> PORT ... 
Invalid PORT.
Retrying.

So, I'm at least cursorily happy with this initial negative test.
Perhaps I'll post it in the l7 datapath tests once this series is merged
(or Darrell can post his own since I think he's respinning).

Hope this helps.  :-)

>> I'm trying to figure out whether I need to do a detailed review myself
>> or just take your acks.
>
> Well, I have never heard anyone say fewer eyes was better.
>
>> Thanks,
>>
>> Ben.
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [patch v2 0/3] conntrack: Alg improvements.

[Aaron Conole]

Ben Pfaff <b...@ovn.org> writes:

> On Mon, Nov 27, 2017 at 06:11:42PM -0500, Aaron Conole wrote:
>> Darrell Ball <dlu...@gmail.com> writes:
>> 
>> > Some refactoring of alg support is done.
>> > Also algs are disabled by default unless an alg specifier
>> > is supplied; this allows for enhanced security and matches
>> > later kernels.
>> > Another change to allow for non-standard alg conntrol
>> > port specification.
>> >  
>> >
>> > Darrell Ball (3):
>> >   conntrack: Refactor algs.
>> >   conntrack: Allow specified alg port numbers.
>> >   conntrack: Disable algs by default.
>> >
>> >  lib/conntrack.c| 168 
>> > ++---
>> >  lib/conntrack.h|   8 +--
>> >  lib/dpif-netdev.c  |   4 +-
>> >  tests/test-conntrack.c |   6 +-
>> >  4 files changed, 127 insertions(+), 59 deletions(-)
>> 
>> For the series -
>> 
>> Acked-by: Aaron Conole <acon...@redhat.com>
>
> Hi Aaron.  Thank you for reviewing--it is time consuming work and does
> not get enough credit.  Can you describe your review process a little?

Sure.  For this, the original work was at:
  https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/341035.html
  https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/341037.html

Darrel and I had some discussions there, and I asked for this to be
split out.  Additionally, I had noticed while reviewing that it didn't
seem non-standard ports were covered for helpers, so asked that such a
change be folded in (and Darrell did).

For the code:
I applied the code, and didn't see anything obviously wrong at the end
(although I did the whole series at once, in the v2 rather than doing
patch-at-a-time).  I could still read through the conntrack code, and
presumably I am able to follow what is happening.

Additionally, once applied, I ran `./utilities/checkpatch.py -3` to
ensure that nothing slipped by.

For the functionality:
I ran the l7 tests via `make check-system-userspace`, and saw that the
l7 ftp tests were passing.  This will require installation of pyftpdlib
and tftpy to execute properly (otherwise they'll be skipped).  Those
tests are explicitly using 'alg=', so should still pass (and they do).

I didn't check anything with manual sets of flows.  I will do that now,
though since outlining this makes me realize I never checked the
'negative' case (meaning assume that no alg= is given, ensure that the
helper really isn't assigned).  I can report back on that tomorrow, if
it makes sense.

> I'm trying to figure out whether I need to do a detailed review myself
> or just take your acks.

Well, I have never heard anyone say fewer eyes was better.

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

Re: [ovs-dev] [patch v1 4/4] conntrack: Change established state to match kernel.

[Aaron Conole]

Lots to digest - responses below

Jan Scheurich  writes:

> Hi Darrel,
> Let me try respond to your points below.
> Regards, Jan
>> -Original Message-
>> From: Darrell Ball [mailto:db...@vmware.com]
>> Sent: Thursday, 30 November, 2017 01:33
>> 
>> The idea of creating an “Conntrack Established state” specific to
>> each protocol layer, as you propose, does not adhere to any protocol
>> specifications that I am aware of.
>> 
>> 1/ UDP and ICMP do not even have such a concept as “Established”
>> connection, so having those specific protocols track “Conntrack
>> Established state” has no technical basis.
>> 
>> 2/ Even if we somehow ignored UDP and ICMP, which we cannot, TCP end
>> to end connection established state is based on a 3-WAY handshake.
>> You propose to have a “Conntrack Established state” based on a 2-way
>> handshake and build knowledge of the kernel derived Conntrack
>> Established state into the TCP state machine itself.
>> I think that is wrong as the TCP state machine should not know
>> anything about the arbitrary “kernel defined established conntrack
>> state”, based on a 2-way handshake or any other Conntrack
>> state. These are different layers.
>> Moreover, it is not even necessary, since the TCP state machine
>> generically tracks packets as valid (as does UDP and ICMP) including
>> replies.
>> So (valid && reply) tells us if we have a valid reply, in a protocol 
>> agnostic manner.
> After having re-read the kernel conntrack specification for userland
> states in http://www.iptables.info/en/connection-state.html (table
> 7-1), I tend to agree with you that the generic definition of
> "established" in the main conntrack module as "valid reply packet
> seen" would be in line with kernel semantics. And we would acknowledge
> your corresponding patch.
>> 3/ This brings us back to the question of bringing userspace
>> conntrack established state in line with the kernel conntrack
>> definition of established.
>> 
>> We don’t see a requirement of blindly copying the kernel; proper
>> modelling and technical merit is more important.
>> The discussion around points “1” and “2” above make this all the more clear.
>> 
>> The kernel uses an arbitrary definition of “conntrack established”
>> based on having received a reply packet and at a midpoint b/w the 2
>> endpoints.
> The Linux kernel definition of ESTABLISHED may be arbitrary and not
> entirely without problems in some corner cases, but we have to admit
> that it is well known and has proven to be useful in very many
> deployments of Linux conntrack at the heart of a stateful firewall
> within an OVS context, but more generally outside OVS. So it has set a
> de-facto standard.
>> The userspace connection tracker defines Established as any packet
>> that hits an existing conntrack entry.
>> Everyone thinks this definition is semantically correct and
>> intuitive. I also think that even you agree with this, based on your
>> previous e-mails and discussions.
>> The existence of a conntrack entry vs none is the key difference
>> that should be tracked for EST and this is exactly what the
>> userspace connection tracker does.
> The userspace conntrack definition of ESTABLISHED would be simple and
> logical, if we were starting from a clean slate. Unfortunately we
> aren't. The new semantics clashes with the Linux de-facto standard and
> the OVS legacy. In my eyes that is where simplicity ends and we create
> problems for users and developers.
> I would also argue that the userspace definition is just as arbitrary
> as the Linux kernel. It is different, perhaps slightly simpler, but I
> wouldn't say it is more correct than the kernel.
>> 
>> Furthermore, as discussed on another thread, any pipeline written
>> properly, where the commit of a conntrack entry happens in the
>> trusted direction, will never observe a difference b/w kernel and
>> userspace EST, since forward direction packets in the trusted
>> direction are always trusted and should not be subject to drop
>> whether NEW or EST.
>> 
>> Hence, I think we should keep userspace conntrack EST as is for now.
>> We can always revisit if we find differences w.r.t. real/proper conntrack 
>> pipelines.
>> Don’t be surprised, if I look at a pipeline and say it is not proper
>> with a reason provided, as I have done earlier. I might even say
>> something like “since your VM accepts all incoming traffic without
>> filtering, why do you even use conntrack rules for that VM”, for
>> example.
>> I know there is an infinite number of broken conntrack pipelines that can be 
>> written.
> I don't think we should lead this discussion based on whether a
> specific example conntrack pipeline is considered "proper" or
> not. Features that OVS offers to its users should have a well-defined
> semantics, no matter what datapath is present and how users choose to
> use them.
> If OVS cannot guarantee well-defined semantics across datapaths, it
> should warn users so that 

Re: [ovs-dev] [PATCH v4 1/3] dpif-netdev: Refactor PMD performance into dpif-netdev-perf

[Aaron Conole]

Darrell Ball <db...@vmware.com> writes:

> On 12/18/17, 2:41 PM, "Jan Scheurich" <jan.scheur...@ericsson.com> wrote:
>
> Hi Aaron,
> Thanks for the review. Answers in-line.
> Regards, Jan
> 
> > -Original Message-
> > From: Aaron Conole [mailto:acon...@redhat.com]
> > Sent: Monday, 18 December, 2017 19:41
> > To: Jan Scheurich <jan.scheur...@ericsson.com>
> > Cc: d...@openvswitch.org
> > Subject: Re: [ovs-dev] [PATCH v4 1/3] dpif-netdev: Refactor PMD 
> performance into dpif-netdev-perf
> > 
> > Hi Jan,
> > 
> > Jan Scheurich <jan.scheur...@ericsson.com> writes:
> > 
> > > Add module dpif-netdev-perf to host all PMD performance-related
> > > data structures and functions in dpif-netdev. Refactor the PMD
> > > stats handling in dpif-netdev and delegate whatever possible into
> > > the new module, using clean interfaces to shield dpif-netdev from
> > > the implementation details. Accordingly, the all PMD statistics
> > > members are moved from the main struct dp_netdev_pmd_thread into
> > > a dedicated member of type struct pmd_perf_stats.
> > >
> > > Include Darrel's prior refactoring of PMD stats contained in
> > > [PATCH v5,2/3] dpif-netdev: Refactor some pmd stats:
> > >
> > > 1. The cycles per packet counts are now based on packets
> > > received rather than packet passes through the datapath.
> > >
> > > 2. Packet counters are now kept for packets received and
> > > packets recirculated. These are kept as separate counters for
> > > maintainability reasons. The cost of incrementing these counters
> > > is negligible.  These new counters are also displayed to the user.
> > >
> > > 3. A display statistic is added for the average number of
> > > datapath passes per packet. This should be useful for user
> > > debugging and understanding of packet processing.
> > >
> > > 4. The user visible 'miss' counter is used for successful upcalls,
> > > rather than the sum of sucessful and unsuccessful upcalls. Hence,
> > > this becomes what user historically understands by OVS 'miss upcall'.
> > > The user display is annotated to make this clear as well.
> > >
> > > 5. The user visible 'lost' counter remains as failed upcalls, but
> > > is annotated to make it clear what the meaning is.
> > >
> > > 6. The enum pmd_stat_type is annotated to make the usage of the
> > > stats counters clear.
> > >
> > > 7. The subtable lookup stats is renamed to make it clear that it
> > > relates to masked lookups.
> > >
> > > 8. The PMD stats test is updated to handle the new user stats of
> > > packets received, packets recirculated and average number of datapath
> > > passes per packet.
> > >
> > > On top of that introduce a "-pmd " option to the PMD info
> > > commands to filter the output for a single PMD.
> > >
> > > Signed-off-by: Jan Scheurich <jan.scheur...@ericsson.com>
> > > Co-authored-by: Darrell Ball <dlu...@gmail.com>
> > > Signed-off-by: Darrell Ball <dlu...@gmail.com>
> > > ---
> > 
> > I'm really happy to see this series making progress.  I think it's
> > extremely useful.
> > 
> > Some comments inline.
> > 
> > >  lib/automake.mk|   2 +
> > >  lib/dpif-netdev-perf.c |  66 ++
> > >  lib/dpif-netdev-perf.h | 123 ++
> > >  lib/dpif-netdev.c  | 330 
> -
> > >  tests/pmd.at   |  22 ++--
> > >  5 files changed, 339 insertions(+), 204 deletions(-)
> > >  create mode 100644 lib/dpif-netdev-perf.c
> > >  create mode 100644 lib/dpif-netdev-perf.h
> > >
> > > diff --git a/lib/automake.mk b/lib/automake.mk
> > > index effe5b5..0b8df0e 100644
> > > --- a/lib/automake.mk
> > > +++ b/lib/automake.mk
> > > @@ -80,6 +80,8 @@ lib_libopenvswitch_la_SOURCES = \
> > >   lib/dpdk.h \
> > >   lib/dpif-netdev.c \
> > >   lib/dpif-netdev.h \
> > > + lib/dpif-netdev-perf.c \
> > > + lib/dpif-netdev-

Re: [ovs-dev] [PATCH v4 1/3] dpif-netdev: Refactor PMD performance into dpif-netdev-perf

[Aaron Conole]

Hi Jan,

Jan Scheurich  writes:

> Add module dpif-netdev-perf to host all PMD performance-related
> data structures and functions in dpif-netdev. Refactor the PMD
> stats handling in dpif-netdev and delegate whatever possible into
> the new module, using clean interfaces to shield dpif-netdev from
> the implementation details. Accordingly, the all PMD statistics
> members are moved from the main struct dp_netdev_pmd_thread into
> a dedicated member of type struct pmd_perf_stats.
>
> Include Darrel's prior refactoring of PMD stats contained in
> [PATCH v5,2/3] dpif-netdev: Refactor some pmd stats:
>
> 1. The cycles per packet counts are now based on packets
> received rather than packet passes through the datapath.
>
> 2. Packet counters are now kept for packets received and
> packets recirculated. These are kept as separate counters for
> maintainability reasons. The cost of incrementing these counters
> is negligible.  These new counters are also displayed to the user.
>
> 3. A display statistic is added for the average number of
> datapath passes per packet. This should be useful for user
> debugging and understanding of packet processing.
>
> 4. The user visible 'miss' counter is used for successful upcalls,
> rather than the sum of sucessful and unsuccessful upcalls. Hence,
> this becomes what user historically understands by OVS 'miss upcall'.
> The user display is annotated to make this clear as well.
>
> 5. The user visible 'lost' counter remains as failed upcalls, but
> is annotated to make it clear what the meaning is.
>
> 6. The enum pmd_stat_type is annotated to make the usage of the
> stats counters clear.
>
> 7. The subtable lookup stats is renamed to make it clear that it
> relates to masked lookups.
>
> 8. The PMD stats test is updated to handle the new user stats of
> packets received, packets recirculated and average number of datapath
> passes per packet.
>
> On top of that introduce a "-pmd " option to the PMD info
> commands to filter the output for a single PMD.
>
> Signed-off-by: Jan Scheurich 
> Co-authored-by: Darrell Ball 
> Signed-off-by: Darrell Ball 
> ---

I'm really happy to see this series making progress.  I think it's
extremely useful.

Some comments inline.

>  lib/automake.mk|   2 +
>  lib/dpif-netdev-perf.c |  66 ++
>  lib/dpif-netdev-perf.h | 123 ++
>  lib/dpif-netdev.c  | 330 
> -
>  tests/pmd.at   |  22 ++--
>  5 files changed, 339 insertions(+), 204 deletions(-)
>  create mode 100644 lib/dpif-netdev-perf.c
>  create mode 100644 lib/dpif-netdev-perf.h
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index effe5b5..0b8df0e 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -80,6 +80,8 @@ lib_libopenvswitch_la_SOURCES = \
>   lib/dpdk.h \
>   lib/dpif-netdev.c \
>   lib/dpif-netdev.h \
> + lib/dpif-netdev-perf.c \
> + lib/dpif-netdev-perf.h \
>   lib/dpif-provider.h \
>   lib/dpif.c \
>   lib/dpif.h \
> diff --git a/lib/dpif-netdev-perf.c b/lib/dpif-netdev-perf.c
> new file mode 100644
> index 000..b198fd3
> --- /dev/null
> +++ b/lib/dpif-netdev-perf.c
> @@ -0,0 +1,66 @@
> +/*
> + * Copyright (c) 2017 Ericsson AB.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include 
> +
> +#ifdef DPDK_NETDEV
> +#include 
> +#endif
> +
> +#include "openvswitch/dynamic-string.h"
> +#include "openvswitch/vlog.h"
> +#include "dpif-netdev-perf.h"
> +#include "timeval.h"
> +
> +VLOG_DEFINE_THIS_MODULE(pmd_perf);
> +
> +void
> +pmd_perf_stats_init(struct pmd_perf_stats *s) {
> +memset(s, 0 , sizeof(*s));
> +s->start_ms = time_msec();
> +}
> +
> +void
> +pmd_perf_read_counters(struct pmd_perf_stats *s,
> +   uint64_t stats[PMD_N_STATS])
> +{
> +uint64_t val;
> +
> +/* These loops subtracts reference values ('*_zero') from the counters.
> + * Since loads and stores are relaxed, it might be possible for a 
> '*_zero'

Kind of a nit, but I think that should read .zero[*] or ->zero[*].  No
big deal one way or the other.

> + * value to be more recent than the current value we're reading from the
> + * counter.  This is not a big problem, since these numbers are not
> + * supposed to be too accurate, but we should at least make sure that
> + * the result is 

Re: [ovs-dev] [PATCH] netdev-dpdk: defer MTU set after interface start

[Aaron Conole]

"Stokes, Ian"  writes:

>> > The issue only arises with the qede PMD and 67fe6d635193
>> > ("netdev-dpdk: use rte_eth_dev_set_mtu.")
>>
>> I had some more time to look at this today but this patch will break
>> OVS DPDK for existing supported DPDK ports during testing.
>>
>> I tested with an XL710 and the MTU will fail to apply, the device
>> must be stopped before configuration changes can be applied such as
>> MTU. See log message below
>>
>> 2017-11-28T17:13:50Z|00086|dpdk|ERR|i40e_dev_mtu_set(): port 0 must
>> be stopped before configuration
>> 2017-11-28T17:13:50Z|00087|netdev_dpdk|ERR|Interface dpdk0 MTU
>> (1500) setup error: Device or resource busy
>>
>> Did you come across it in your testing? I guess you didn’t see this
>> for QEDE pmd. In my case the DPDK devices will simply fail to add to
>> the
>> bridge.
>>
>> As is, the patch would not be acceptable as its breaking existing
>> functionality. It would have to be reworked to configure for device
>> that
>> cannot reconfigure when busy.
>>
>> Thanks
>> Ian
>
> I fully support the original decision to switch to using
> rte_dev_set_mtu() in OVS. The prior approach setting max_rx_pkt_len in
> rte_eth_dev_configure() was non-portable as that field has no
> well-defined semantics and its relation to the MTU size is different
> for almost every PMD.
>
> I had a look at the qede PMD implementation of rte_dev_set_mtu() in
> DPDK 17.05 and 17.11. It assumes that the device must be started
> because qede_set_mtu() unconditionally stops the device before and
> restarts it after changing the MTU value. Given that other PMDs like
> i40e don’t accept it after start, there is no possible way OVS can use
> rte_dev_set_mtu() that will work with all drivers.
>
> I think it is a weakness of the rte_ethdev API that it does not
> specify clearly when API functions like rte_dev_set_mtu() may be
> called. From the documentation of error -EBUSY one can guess that the
> intention was to optionally support it when the device is
> started. Implicitly one could conclude that it MUST be supported when
> the device stopped. That is logical and also what most PMDs do.
>
> I would say the qede PMD should be fixed. It should accept the
> rte_dev_set_mtu() at any time between rte_eth_dev_configure() and
> rte_eth_dev_start() (and optionally also after start).
>
> BR, Jan
>
> [IS] Thanks for doing this follow up Jan, it’s really valuable in my opinion.
>
> It’s definitely something that should be raised to the DPDK community
> as conceivably if there is no set process to follow we could see
> varying behavior from PMD device to device. I’m happy to take this and
> raise it to the DPDK community.

Agreed that the understanding of when/how this setter can be called is
confusing.  I contacted Harish Patil (who works on that driver for the
DPDK side) about this, and think that there's a followup to this thread
coming.

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

Re: [ovs-dev] [PATCH v3 1/3] dpif-netdev: Refactor PMD performance into dpif-netdev-perf

[Aaron Conole]

Jan Scheurich  writes:

> Hi Billy,
>
> My patches frequently get corrupted by our email system. When I submit
> the v4 of the series adapted to the reverted dp_netdev_pmd_thread
> struct changes I will try out another method of sending them.
>
> For now, could you please try the attached patch files? For me they
> apply cleanly on today's master. Haven't tested them yet.

I wanted to follow up on this, but it seems I don't have any attachments
for this email.  Did my mail client break something?

> I'll fix the white-space errors and line lengths issues in the next
> version. More answers in-line.
>
> Thanks, Jan
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath: Enforce matching of kernel releases on loading openvswitch.ko

[Aaron Conole]

Yifeng Sun  writes:

> Deployment and upgrade failure is quite often caused by that openvswitch.ko 
> was
> built upon kernel x.y.z-release-A while it is loaded into a running kernel
> of x.y.z-release-B. This patch proposes to enforce the matching of the two
> kernel release numbers at the moment of deployment and upgrading.
>
> Signed-off-by: Yifeng Sun 
> ---

Is this to circumvent CONFIG_MODVERSIONS=n?

This seems like a situation that shouldn't happen.  When upgrading, the
installed version (make modules_install) should be matched (ie: a
modprobe won't work because the kernel `uname -r` directory changed).
Otherwise, a new build should be done by the developer that changed out
their kernel (because they are manually building OvS modules and
installing from the source tree).

I'm probably not understanding the use case.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath: Enforce matching of kernel releases on loading openvswitch.ko

[Aaron Conole]

Yifeng Sun <pkusunyif...@gmail.com> writes:

> Thanks for your review.
>
> Since CONFIG_MODVERSIONS is default to 'y' in most cases, I'd say this
> is to circumvent CONFIG_MODVERSIONS=y.
>
> This happened when redhat kernel was upgraded from
> 3.10.0-514.el7.x86_64 to 3.10.0-693.1.1.el7.x86_64. When 693 uses the
> openvswitch.ko built for 514, some error happened.

Maybe you want something like a dkms module?  I'm not sure.  I do know
that RHEL ships an openvswitch.ko which is kept up to date with
upstream, and should have been shipped with the kernel.  Is that not the
case?

> On Wed, Dec 13, 2017 at 10:24 AM, Aaron Conole <acon...@redhat.com> wrote:
>
>  Yifeng Sun <pkusunyif...@gmail.com> writes:
>
>  > Deployment and upgrade failure is quite often caused by that 
> openvswitch.ko was
>  > built upon kernel x.y.z-release-A while it is loaded into a running kernel
>  > of x.y.z-release-B. This patch proposes to enforce the matching of the two
>  > kernel release numbers at the moment of deployment and upgrading.
>  >
>  > Signed-off-by: Yifeng Sun <pkusunyif...@gmail.com>
>  > ---
>
>  Is this to circumvent CONFIG_MODVERSIONS=n?
>
>  This seems like a situation that shouldn't happen.  When upgrading, the
>  installed version (make modules_install) should be matched (ie: a
>  modprobe won't work because the kernel `uname -r` directory changed).
>  Otherwise, a new build should be done by the developer that changed out
>  their kernel (because they are manually building OvS modules and
>  installing from the source tree).
>
>  I'm probably not understanding the use case.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/9] tests: Convert ovsdb-monitor-sort utility from Perl to Python.

[Aaron Conole]

Hi Ben,

Ben Pfaff  writes:

> Perl is unfashionable and Python is more widely available and understood,
> so this commit converts one of the OVS uses of Perl into Python.
>
> Signed-off-by: Ben Pfaff 
> ---
>  tests/automake.mk   |  2 +-
>  tests/ovsdb-monitor-sort.pl | 52 ---
>  tests/ovsdb-monitor-sort.py | 85 
> +
>  tests/ovsdb-monitor.at  |  4 +--
>  4 files changed, 88 insertions(+), 55 deletions(-)
>  delete mode 100755 tests/ovsdb-monitor-sort.pl
>  create mode 100755 tests/ovsdb-monitor-sort.py
>
> diff --git a/tests/automake.mk b/tests/automake.mk
> index 3ca60e2ea450..1ea08fef850d 100644
> --- a/tests/automake.mk
> +++ b/tests/automake.mk
> @@ -290,7 +290,6 @@ $(srcdir)/package.m4: $(top_srcdir)/configure.ac
>  noinst_PROGRAMS += tests/test-ovsdb
>  tests_test_ovsdb_SOURCES = tests/test-ovsdb.c
>  nodist_tests_test_ovsdb_SOURCES = tests/idltest.c tests/idltest.h
> -EXTRA_DIST += tests/ovsdb-monitor-sort.pl
>  tests_test_ovsdb_LDADD = ovsdb/libovsdb.la lib/libopenvswitch.la
>  
>  noinst_PROGRAMS += tests/test-lib
> @@ -380,6 +379,7 @@ tests_test_type_props_SOURCES = tests/test-type-props.c
>  # Python tests.
>  CHECK_PYFILES = \
>   tests/appctl.py \
> + tests/ovsdb-monitor-sort.py \
>   tests/test-daemon.py \
>   tests/test-json.py \
>   tests/test-jsonrpc.py \
> diff --git a/tests/ovsdb-monitor-sort.pl b/tests/ovsdb-monitor-sort.pl
> deleted file mode 100755
> index 24f3ffcd6162..
> --- a/tests/ovsdb-monitor-sort.pl
> +++ /dev/null
> @@ -1,52 +0,0 @@
> -#! /usr/bin/perl
> -
> -use strict;
> -use warnings;
> -
> -# Breaks lines read from  into groups using blank lines as
> -# group separators, then sorts lines within the groups for
> -# reproducibility.
> -
> -sub compare_lines {
> -my ($a, $b) = @_;
> -
> -my $u = '[0-9a-fA-F]';
> -my $uuid_re = "${u}{8}-${u}{4}-${u}{4}-${u}{4}-${u}{12}";
> -if ($a =~ /^$uuid_re/) {
> -if ($b =~ /^$uuid_re/) {
> -return substr($a, 36) cmp substr($b, 36);
> -} else {
> -return 1;
> -}
> -} elsif ($b =~ /^$uuid_re/) {
> -return -1;
> -} else {
> -return $a cmp $b;
> -}
> -}
> -
> -sub output_group {
> -my (@group) = @_;
> -print "$_\n" foreach sort { compare_lines($a, $b) } @group;
> -}
> -
> -if ("$^O" eq "msys") {
> -$/ = "\r\n";
> -}
> -my @group = ();
> -while () {
> -chomp;
> -if ($_ eq '') {
> -output_group(@group);
> -@group = ();
> -print "\n";
> -} else {
> -if (/^,/ && @group) {
> -$group[$#group] .= "\n" . $_;
> -} else {
> -push(@group, $_);
> -}
> -}
> -}
> -
> -output_group(@group) if @group;
> diff --git a/tests/ovsdb-monitor-sort.py b/tests/ovsdb-monitor-sort.py
> new file mode 100755
> index ..c538d9d6b573
> --- /dev/null
> +++ b/tests/ovsdb-monitor-sort.py
> @@ -0,0 +1,85 @@
> +#! /usr/bin/env python
> +
> +# Breaks lines read from stdin into groups using blank lines as
> +# group separators, then sorts lines within the groups for
> +# reproducibility.
> +
> +import re
> +import sys
> +
> +
> +# This is copied out of the Python Sorting HOWTO at
> +# https://docs.python.org/3/howto/sorting.html#sortinghowto
> +def cmp_to_key(mycmp):
> +'Convert a cmp= function into a key= function'
> +class K:

I get a flake8 complaint for this - I think it should be:

 class K(object):
 ...

> +
> +def __init__(self, obj, *args):
> +self.obj = obj
> +
> +def __lt__(self, other):
> +return mycmp(self.obj, other.obj) < 0
> +
> +def __gt__(self, other):
> +return mycmp(self.obj, other.obj) > 0
> +
> +def __eq__(self, other):
> +return mycmp(self.obj, other.obj) == 0
> +
> +def __le__(self, other):
> +return mycmp(self.obj, other.obj) <= 0
> +
> +def __ge__(self, other):
> +return mycmp(self.obj, other.obj) >= 0
> +
> +def __ne__(self, other):
> +return mycmp(self.obj, other.obj) != 0
> +
> +return K
> +
> +
> +u = '[0-9a-fA-F]'
> +uuid_re = re.compile(r'%s{8}-%s{4}-%s{4}-%s{4}-%s{12}' % ((u,) * 5))
> +
> +
> +def cmp(a, b):
> +return (a > b) - (a < b)
> +
> +
> +def compare_lines(a, b):
> +if uuid_re.match(a):
> +if uuid_re.match(b):
> +return cmp(a[36:], b[36:])
> +else:
> +return 1
> +elif uuid_re.match(b):
> +return -1
> +else:
> +return cmp(a, b)
> +
> +
> +def output_group(group, dst):
> +for x in sorted(group, key=cmp_to_key(compare_lines)):
> +dst.write(x)
> +
> +
> +def ovsdb_monitor_sort(src, dst):
> +group = []
> +while True:
> +line = src.readline()
> +if not line:
> +break
> +if line.rstrip() == '':
> +output_group(group, dst)

Re: [ovs-dev] [PATCH 7/9] tests: Convert dot2pic build tool from Perl to Python.

[Aaron Conole]

Hi Ben,

Ben Pfaff  writes:

> Perl is unfashionable and Python is more widely available and understood,
> so this commit converts one of the OVS uses of Perl into Python.
>
> Signed-off-by: Ben Pfaff 
> ---
>  ovn/automake.mk  |   4 +-
>  ovsdb/dot2pic| 155 
> +++
>  vswitchd/automake.mk |   2 +-
>  vtep/automake.mk |   2 +-
>  4 files changed, 100 insertions(+), 63 deletions(-)
>
> diff --git a/ovn/automake.mk b/ovn/automake.mk
> index c5925e9285ac..b33112ef14e5 100644
> --- a/ovn/automake.mk
> +++ b/ovn/automake.mk
> @@ -11,7 +11,7 @@ if HAVE_DOT
>  ovn/ovn-sb.gv: ovsdb/ovsdb-dot.in ovn/ovn-sb.ovsschema
>   $(AM_V_GEN)$(OVSDB_DOT) --no-arrows $(srcdir)/ovn/ovn-sb.ovsschema > $@
>  ovn/ovn-sb.pic: ovn/ovn-sb.gv ovsdb/dot2pic
> - $(AM_V_GEN)(dot -T plain < ovn/ovn-sb.gv | $(PERL) 
> $(srcdir)/ovsdb/dot2pic -f 3) > $@.tmp && \
> + $(AM_V_GEN)(dot -T plain < ovn/ovn-sb.gv | $(PYTHON) 
> $(srcdir)/ovsdb/dot2pic -f 3) > $@.tmp && \
>   mv $@.tmp $@
>  OVN_SB_PIC = ovn/ovn-sb.pic
>  OVN_SB_DOT_DIAGRAM_ARG = --er-diagram=$(OVN_SB_PIC)
> @@ -45,7 +45,7 @@ if HAVE_DOT
>  ovn/ovn-nb.gv: ovsdb/ovsdb-dot.in ovn/ovn-nb.ovsschema
>   $(AM_V_GEN)$(OVSDB_DOT) --no-arrows $(srcdir)/ovn/ovn-nb.ovsschema > $@
>  ovn/ovn-nb.pic: ovn/ovn-nb.gv ovsdb/dot2pic
> - $(AM_V_GEN)(dot -T plain < ovn/ovn-nb.gv | $(PERL) 
> $(srcdir)/ovsdb/dot2pic -f 3) > $@.tmp && \
> + $(AM_V_GEN)(dot -T plain < ovn/ovn-nb.gv | $(PYTHON) 
> $(srcdir)/ovsdb/dot2pic -f 3) > $@.tmp && \
>   mv $@.tmp $@
>  OVN_NB_PIC = ovn/ovn-nb.pic
>  OVN_NB_DOT_DIAGRAM_ARG = --er-diagram=$(OVN_NB_PIC)
> diff --git a/ovsdb/dot2pic b/ovsdb/dot2pic
> index d682be5f9610..51d454b62e18 100755
> --- a/ovsdb/dot2pic
> +++ b/ovsdb/dot2pic
> @@ -1,6 +1,6 @@
> -#! /usr/bin/perl
> +#! /usr/bin/env python
>  
> -# Copyright (c) 2009, 2010, 2011, 2013 Nicira, Inc.
> +# Copyright (c) 2009, 2010, 2011, 2013, 2017 Nicira, Inc.
>  #
>  # Licensed under the Apache License, Version 2.0 (the "License");
>  # you may not use this file except in compliance with the License.
> @@ -14,67 +14,104 @@
>  # See the License for the specific language governing permissions and
>  # limitations under the License.
>  
> -use strict;
> -use warnings;
> +import getopt
> +import sys
>  
> -use Getopt::Long;
> +def dot2pic(src, dst):
> +scale = 1.0
> +while True:
> +line = src.readline()
> +if not line:
> +break
>  
> -my $font_scale = 0;
> -GetOptions("f=i" => \$font_scale) || exit 1;
> +words = line.split()
> +command = words[0]
> +if command == 'graph':
> +scale = float(words[1])
> +elif command == 'node':
> +name = words[1]
> +x = float(words[2])
> +y = float(words[3])
> +width = float(words[4])
> +height = float(words[5])
> +label, style, shape, color, fillcolor = words[6:11]
> +x *= scale
> +y *= scale
> +width *= scale
> +height *= scale
> +dst.write("linethick = %f;\n" % (0.5 if style == 'bold' else 
> 1.0))
> +dst.write('box at %f,%f wid %f height %f "%s"\n'
> +  % (x, y, width, height, name))
> +if style == 'bold':
> +inset = 2.0 / 72.0
> +width -= inset * 2
> +height -= inset * 2
> +dst.write("box at %f,%f wid %f height %f\n"
> +  % (x, y, width, height))
> +elif command == 'edge':
> +tail = words[1]
> +head = words[2]
> +n = int(words[3])
>  
> -my ($scale) = 1;
> -printf ".ps %+d\n", -$font_scale if $font_scale;
> -print ".PS\n";
> -print "linethick = 1;\n";
> -while (<>) {
> -if (/^graph/) {
> -(undef, $scale) = split;
> -} elsif (/^node/) {
> -my (undef, $name, $x, $y, $width, $height, $label, $style, $shape, 
> $color, $fillcolor) = split;
> -$x *= $scale;
> -$y *= $scale;
> -$width *= $scale;
> -$height *= $scale;
> -print "linethick = ", ($style eq 'bold' ? 0.5 : 1.0), ";\n";
> -print "box at $x,$y wid $width height $height \"$name\"\n";
> -if ($style eq 'bold') {
> -my $inset = 2.0 / 72.0;
> -$width -= $inset * 2;
> -$height -= $inset * 2;
> -print "box at $x,$y wid $width height $height\n";
> -}
> -} elsif (/edge/) {
> -my (undef, $tail, $head, $n, $rest) = split(' ', $_, 5);
> -my @xy;
> -for (1...$n) {
> -my ($x, $y);
> -($x, $y, $rest) = split(' ', $rest, 3);
> -push(@xy, [$x * $scale, $y * $scale]);
> -}
> -my ($label, $xl, $yl);
> -if (scalar(my @junk = split(' ', $rest)) > 2) {
> -if ($rest =~ s/^"([^"]*)"\s+//) {
> -$label = 

Re: [ovs-dev] [PATCH 5/9] tests: Convert soexpand build tool from Perl to Python.

[Aaron Conole]

Ben Pfaff  writes:

> Perl is unfashionable and Python is more widely available and understood,
> so this commit converts one of the OVS uses of Perl into Python.
>
> Signed-off-by: Ben Pfaff 
> ---
>  Makefile.am   |  4 +--
>  build-aux/automake.mk |  3 +-
>  build-aux/soexpand.pl | 40 -
>  build-aux/soexpand.py | 97 
> +++
>  4 files changed, 101 insertions(+), 43 deletions(-)
>  delete mode 100644 build-aux/soexpand.pl
>  create mode 100755 build-aux/soexpand.py
>
> diff --git a/Makefile.am b/Makefile.am
> index c82a9e21ec36..11e2e6d21005 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -88,7 +88,7 @@ EXTRA_DIST = \
>   build-aux/dist-docs \
>   build-aux/dpdkstrip.py \
>   build-aux/sodepends.pl \
> - build-aux/soexpand.pl \
> + build-aux/soexpand.py \
>   build-aux/xml2nroff \
>   $(MAN_FRAGMENTS) \
>   $(MAN_ROOTS) \
> @@ -144,7 +144,7 @@ ro_shell = printf '\043 Generated automatically -- do not 
> modify!-*- buffer-
>  
>  SUFFIXES += .in
>  .in:
> - $(AM_V_GEN)$(PERL) $(srcdir)/build-aux/soexpand.pl -I$(srcdir) < $< | \
> + $(AM_V_GEN)$(PYTHON) $(srcdir)/build-aux/soexpand.py -I$(srcdir) < $< | 
> \
> $(PYTHON) $(srcdir)/build-aux/dpdkstrip.py $(DPDKSTRIP_FLAGS) | \
> sed \
>   -e 's,[@]PKIDIR[@],$(PKIDIR),g' \
> diff --git a/build-aux/automake.mk b/build-aux/automake.mk
> index c0553e6edffb..1003144fd664 100644
> --- a/build-aux/automake.mk
> +++ b/build-aux/automake.mk
> @@ -1,4 +1,5 @@
>  # This file is purely used for checking the style of the python build tools.
>  FLAKE8_PYFILES += \
>  $(srcdir)/build-aux/xml2nroff \
> -build-aux/dpdkstrip.py
> +build-aux/dpdkstrip.py \
> +build-aux/soexpand.py
> diff --git a/build-aux/soexpand.pl b/build-aux/soexpand.pl
> deleted file mode 100644
> index 216256451a4d..
> --- a/build-aux/soexpand.pl
> +++ /dev/null
> @@ -1,40 +0,0 @@
> -# Copyright (c) 2008 Nicira, Inc.
> -#
> -# Licensed under the Apache License, Version 2.0 (the "License");
> -# you may not use this file except in compliance with the License.
> -# You may obtain a copy of the License at:
> -#
> -# http://www.apache.org/licenses/LICENSE-2.0
> -#
> -# Unless required by applicable law or agreed to in writing, software
> -# distributed under the License is distributed on an "AS IS" BASIS,
> -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> -# See the License for the specific language governing permissions and
> -# limitations under the License.
> -
> -use strict;
> -use warnings;
> -use Getopt::Long;
> -
> -my ($exit_code) = 0;
> -my (@include_dirs);
> -Getopt::Long::Configure ("bundling");
> -GetOptions("I|include=s" => \@include_dirs) or exit(1);
> -@include_dirs = ('.') if !@include_dirs;
> -OUTER: while () {
> -if (my ($name) = /^\.so (\S+)$/) {
> - foreach my $dir (@include_dirs, '.') {
> - if (open(INNER, "$dir/$name")) {
> - while () {
> - print $_;
> - }
> - close(INNER);
> - next OUTER;
> - }
> - }
> - print STDERR "$name not found in: ", join(' ', @include_dirs), "\n";
> - $exit_code = 1;
> -}
> -print $_;
> -}
> -exit $exit_code;
> diff --git a/build-aux/soexpand.py b/build-aux/soexpand.py
> new file mode 100755
> index ..fe99b461f980
> --- /dev/null
> +++ b/build-aux/soexpand.py
> @@ -0,0 +1,97 @@
> +#! /usr/bin/env python
> +
> +# Copyright (c) 2008, 2017 Nicira, Inc.
> +#
> +# Licensed under the Apache License, Version 2.0 (the "License");
> +# you may not use this file except in compliance with the License.
> +# You may obtain a copy of the License at:
> +#
> +# http://www.apache.org/licenses/LICENSE-2.0
> +#
> +# Unless required by applicable law or agreed to in writing, software
> +# distributed under the License is distributed on an "AS IS" BASIS,
> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> +# See the License for the specific language governing permissions and
> +# limitations under the License.
> +
> +import getopt
> +import os
> +import re
> +import sys
> +
> +
> +def parse_include_dirs():
> +include_dirs = []
> +options, args = getopt.gnu_getopt(sys.argv[1:], 'I:', ['include='])
> +for key, value in options:
> +if key in ['-I', '--include']:
> +include_dirs.append(value)
> +else:
> +assert False
> +
> +include_dirs.append('.')
> +return include_dirs, args
> +
> +
> +def find_file(include_dirs, name):
> +for dir in include_dirs:
> +file = "%s/%s" % (dir, name)
> +try:
> +os.stat(file)
> +return file
> +except IOError as e:

This should just be 'except IOError:'

> +pass
> +sys.stderr.write("%s not found in: %s\n" % (name, ' 
> '.join(include_dirs)))
> +

Re: [ovs-dev] [PATCH 6/9] tests: Convert sodepends build tool from Perl to Python.

[Aaron Conole]

Ben Pfaff  writes:

> Perl is unfashionable and Python is more widely available and understood,
> so this commit converts one of the OVS uses of Perl into Python.
>
> Signed-off-by: Ben Pfaff 
> ---
>  Makefile.am|   6 +--
>  build-aux/automake.mk  |   1 +
>  build-aux/sodepends.pl |  70 -
>  build-aux/sodepends.py | 105 
> +
>  4 files changed, 109 insertions(+), 73 deletions(-)
>  delete mode 100644 build-aux/sodepends.pl
>  create mode 100755 build-aux/sodepends.py
>
> diff --git a/Makefile.am b/Makefile.am
> index 11e2e6d21005..5bcd2919c1b4 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -87,7 +87,7 @@ EXTRA_DIST = \
>   build-aux/calculate-schema-cksum \
>   build-aux/dist-docs \
>   build-aux/dpdkstrip.py \
> - build-aux/sodepends.pl \
> + build-aux/sodepends.py \
>   build-aux/soexpand.py \
>   build-aux/xml2nroff \
>   $(MAN_FRAGMENTS) \
> @@ -394,8 +394,8 @@ endif
>  CLEANFILES += flake8-check
>  
>  include $(srcdir)/manpages.mk
> -$(srcdir)/manpages.mk: $(MAN_ROOTS) build-aux/sodepends.pl
> - @$(PERL) $(srcdir)/build-aux/sodepends.pl -I. -I$(srcdir) $(MAN_ROOTS) 
> >$(@F).tmp
> +$(srcdir)/manpages.mk: $(MAN_ROOTS) build-aux/sodepends.py
> + @$(PYTHON) $(srcdir)/build-aux/sodepends.py -I. -I$(srcdir) 
> $(MAN_ROOTS) >$(@F).tmp
>   @if cmp -s $(@F).tmp $@; then \
> touch $@; \
> rm -f $(@F).tmp; \
> diff --git a/build-aux/automake.mk b/build-aux/automake.mk
> index 1003144fd664..6baafab0e867 100644
> --- a/build-aux/automake.mk
> +++ b/build-aux/automake.mk
> @@ -2,4 +2,5 @@
>  FLAKE8_PYFILES += \
>  $(srcdir)/build-aux/xml2nroff \
>  build-aux/dpdkstrip.py \
> +build-aux/sodepends.py \
>  build-aux/soexpand.py
> diff --git a/build-aux/sodepends.pl b/build-aux/sodepends.pl
> deleted file mode 100644
> index 333d037f2dcf..
> --- a/build-aux/sodepends.pl
> +++ /dev/null
> @@ -1,70 +0,0 @@
> -# Copyright (c) 2008, 2011 Nicira, Inc.
> -#
> -# Licensed under the Apache License, Version 2.0 (the "License");
> -# you may not use this file except in compliance with the License.
> -# You may obtain a copy of the License at:
> -#
> -# http://www.apache.org/licenses/LICENSE-2.0
> -#
> -# Unless required by applicable law or agreed to in writing, software
> -# distributed under the License is distributed on an "AS IS" BASIS,
> -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> -# See the License for the specific language governing permissions and
> -# limitations under the License.
> -
> -use strict;
> -use warnings;
> -use Getopt::Long;
> -
> -our ($exit_code) = 0;
> -
> -our (@include_dirs);
> -Getopt::Long::Configure ("bundling");
> -GetOptions("I|include=s" => \@include_dirs) or exit(1);
> -@include_dirs = ('.') if !@include_dirs;
> -
> -sub find_file {
> -my ($name) = @_;
> -foreach my $dir (@include_dirs, '.') {
> -my $file = "$dir/$name";
> -if (stat($file)) {
> -return $file;
> -}
> -}
> -print STDERR "$name not found in: ", join(' ', @include_dirs), "\n";
> -$exit_code = 1;
> -return;
> -}
> -
> -print "# Generated automatically -- do not modify!-*- buffer-read-only: 
> t -*-\n";
> -for my $toplevel (sort(@ARGV)) {
> -# Skip names that don't end in .in.
> -next if $toplevel !~ /\.in$/;
> -
> -# Open file.
> -my ($fn) = find_file($toplevel);
> -next if !defined($fn);
> -if (!open(OUTER, '<', $fn)) {
> -print "$fn: open: $!\n";
> -$exit_code = 1;
> -next;
> -}
> -
> -my (@dependencies);
> -  OUTER:
> -while () {
> -if (my ($name) = /^\.so (\S+)$/) {
> -push(@dependencies, $name) if find_file($name);
> -}
> -}
> -close(OUTER);
> -
> -my ($output) = $toplevel;
> -$output =~ s/\.in//;
> -
> -print "\n$output:";
> -print " \\\n\t$_" foreach $toplevel, sort(@dependencies);
> -print "\n";
> -print "$_:\n" foreach $toplevel, sort(@dependencies);
> -}
> -exit $exit_code;
> diff --git a/build-aux/sodepends.py b/build-aux/sodepends.py
> new file mode 100755
> index ..fafe900b9965
> --- /dev/null
> +++ b/build-aux/sodepends.py
> @@ -0,0 +1,105 @@
> +#! /usr/bin/env python
> +
> +# Copyright (c) 2008, 2011, 2017 Nicira, Inc.
> +#
> +# Licensed under the Apache License, Version 2.0 (the "License");
> +# you may not use this file except in compliance with the License.
> +# You may obtain a copy of the License at:
> +#
> +# http://www.apache.org/licenses/LICENSE-2.0
> +#
> +# Unless required by applicable law or agreed to in writing, software
> +# distributed under the License is distributed on an "AS IS" BASIS,
> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> +# See the License for the specific language governing permissions and
> +# limitations 

Re: [ovs-dev] [PATCH RFC] netdev-dpdk: Fix device obtain mac address when received first packet in vhost type

[Aaron Conole]

Hi Hailin,

Hailin Chen  writes:

> The stp could not work on netdev-dpdk if network is loop.
> Because the stp protocol negotiates designate port by sending
> BPDU packets which contains MAC address.
> However the device doesn't have MAC address in vhostuser type.
> Thus, function send_bpdu_cb would not send BPDU packets.
>
> This patch will set the MAC for device when received first packet.
>
> Signed-off-by: Hailin Chen 
> ---

Thanks for the patch.

In general, I don't think this is the right approach to deal with this
type of issue.  I believe the problem statement is that OvS bridge is
unaware of the guest MAC address - did I get it right?  In that case, I
would think that a better way to solve this would be to have virtio tell
the mac address of the guest.  I don't recall right now if that's
allowed in the virtio spec, but I do remember some kind of negotiation
features.

I've CC'd Maxime, who is one of the maintainers of the virtio code from
DPDK side.  Perhaps there is an alternate way to solve this.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] ovs-tcpdump error

[Aaron Conole]

"Bodireddy, Bhanuprakash" <bhanuprakash.bodire...@intel.com> writes:

>>Aaron Conole <acon...@redhat.com> writes:
>>
>>> Hi Bhanu,
>>>
>>> "Bodireddy, Bhanuprakash" <bhanuprakash.bodire...@intel.com> writes:
>>>
>>>> Hi,
>>>>
>>>>
>>>>
>>>> ovs-tcpdump throws the below error when trying to capture packets on
>>>> one of the vhostuserports.
>>>>
>>>>
>>>>
>>>> $ ovs-tcpdump -i dpdkvhostuser0
>>>>
>>>>ERROR: Please create an interface called `midpdkvhostuser0`
>>>>
>>>> See your OS guide for how to do this.
>>>>
>>>> Ex: ip link add midpdkvhostuser0 type veth peer name
>>>> midpdkvhostuser02
>>>>
>>>>
>>>>
>>>> $ ip link add midpdkvhostuser0 type veth peer name midpdkvhostuser02
>>>>
>>>>  Error: argument "midpdkvhostuser0" is wrong: "name" too long
>>>>
>>>>
>>>>
>>>> To get around this issue, I have to pass  ‘—mirror-to’ option as below.
>>>>
>>>>
>>>>
>>>> $ ovs-tcpdump -i dpdkvhostuser0 -XX --mirror-to vh0
>>>>
>>>>
>>>>
>>>> Is this due to the length of the port name?  Would be nice to fix this 
>>>> issue.
>>>
>>> Thanks for the detailed write up.
>>>
>>> It is related to the mirror port name length.  The mirror port is
>>> bound by IFNAMSIZ restriction, so it must be 15 characters + nul, and
>>> midpdkvhostuser0 would be 16 + nul.  This is a linux specific
>>> restriction, and it won't be changed because it is very much a well
>>> established UAPI (and changing it will have implications on code not
>>> able to deal with larger sized name buffers).
>>>
>>> I'm not sure how best to fix it.  My concession was the mirror-to
>>> option.  Perhaps there's a better way?
>>
>>Hi Bhanu, I've been thinking about this a bit more.  How about something like
>>the following patch?
>>
>>If you think it's acceptable, I'll submit it formally.
>
> Hi Aaron,
>
> I am on fedora and applied the patch but couldn't verify the fix as I get the 
> below error.
>
> Traceback (most recent call last):
>   File "./utilities/ovs-tcpdump", line 21, in 
> import random.randint
> ImportError: No module named randint
>
> When I slightly change the code to
>
> -import random.randint
> + from random import randint
> ...
> -return "ovsmi%06d" % random.randint(1, 99)
> +return "ovsmi%06d" % randint(1, 99)
>
> I get below error
> Traceback (most recent call last):
>   File "./utilities/ovs-tcpdump", line 478, in 
> main()
>   File "./utilities/ovs-tcpdump", line 419, in main
> mirror_interface = mirror_interface or _make_mirror_name(interface)
> TypeError: 'dict' object is not callable
>
> Why is this so?
>
> - Bhanuprakash.
>

Ugh.  Last time I try to be a hero and write python code without at
least a single try.  I'll just submit it formally after quick testing.
Standby.

>>---
>>
>>diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in index
>>6718c77..76e8a7b 100755
>>--- a/utilities/ovs-tcpdump.in
>>+++ b/utilities/ovs-tcpdump.in
>>@@ -18,6 +18,7 @@ import fcntl
>>
>> import os
>> import pwd
>>+import random.randint
>> import struct
>> import subprocess
>> import sys
>>@@ -39,6 +40,7 @@ except Exception:
>>
>> tapdev_fd = None
>> _make_taps = {}
>>+_make_mirror_name = {}
>>
>>
>> def _doexec(*args, **kwargs):
>>@@ -76,8 +78,16 @@ def _install_tap_linux(tap_name, mtu_value=None):
>> pipe.wait()
>>
>>
>>+def _make_linux_mirror_name(interface_name):
>>+if interface_name.length() > 13:
>>+return "ovsmi%06d" % random.randint(1, 99)
>>+return "mi%s" % interface_name
>>+
>>+
>> _make_taps['linux'] = _install_tap_linux  _make_taps['linux2'] =
>>_install_tap_linux
>>+_make_mirror_name['linux'] = _make_linux_mirror_name
>>+_make_mirror_name['linux2'] = _make_linux_mirror_name
>>
>>
>> def username():
>>@@ -406,7 +416,7 @@ def main():
>> print("TCPDUMP Args: %s" % ' '.join(tcpdargs))
>>
>> ovsdb = OVSDB(db_sock)
>>-mirror_interface = mirror_interface or "mi%s" % interface
>>+mirror_interface = mirror_interface or _make_mirror_name(interface)
>>
>> if sys.platform in _make_taps and \
>>mirror_interface not in netifaces.interfaces():
>>---
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] ovs-tcpdump: handle large interface names on linux

[Aaron Conole]

Linux has a fixed size interface name, which will not change.  This means
that attempts to dump interfaces whose names are larger than the max size
will result in an error making the tap device.

This commit brings a new function.  When the generated name would be too
large, use a random number prefixed by 'ovsmi' instead.

Reported-by: Bhanuprakash Bodireddy <bhanuprakash.bodire...@intel.com>
Signed-off-by: Aaron Conole <acon...@redhat.com>
---
 utilities/ovs-tcpdump.in | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
index 6718c77..91fa14e 100755
--- a/utilities/ovs-tcpdump.in
+++ b/utilities/ovs-tcpdump.in
@@ -18,6 +18,7 @@ import fcntl
 
 import os
 import pwd
+from random import randint
 import struct
 import subprocess
 import sys
@@ -39,6 +40,8 @@ except Exception:
 
 tapdev_fd = None
 _make_taps = {}
+_make_mirror_name = {}
+IFNAMSIZ_LINUX = 15  # this is the max name size, excluding the null byte.
 
 
 def _doexec(*args, **kwargs):
@@ -76,8 +79,16 @@ def _install_tap_linux(tap_name, mtu_value=None):
 pipe.wait()
 
 
+def _make_linux_mirror_name(interface_name):
+if len(interface_name) > IFNAMSIZ_LINUX - 2:
+return "ovsmi%06d" % randint(1, 99)
+return "mi%s" % interface_name
+
+
 _make_taps['linux'] = _install_tap_linux
 _make_taps['linux2'] = _install_tap_linux
+_make_mirror_name['linux'] = _make_linux_mirror_name
+_make_mirror_name['linux2'] = _make_linux_mirror_name
 
 
 def username():
@@ -406,7 +417,10 @@ def main():
 print("TCPDUMP Args: %s" % ' '.join(tcpdargs))
 
 ovsdb = OVSDB(db_sock)
-mirror_interface = mirror_interface or "mi%s" % interface
+if mirror_interface is None:
+mirror_interface = "mi%s" % interface
+if sys.platform in _make_mirror_name:
+mirror_interface = _make_mirror_name[sys.platform](interface)
 
 if sys.platform in _make_taps and \
mirror_interface not in netifaces.interfaces():
-- 
2.9.5

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

[ovs-dev] [CONNTRACK] Discussions at OvS 2017

[Aaron Conole]

(NOTE: This is a resend - I fat-fingered the ovs email.  Apologies to
those who got duplicates).

This email is meant to summarize some of the discussions we had at OvS
conference.

The interest in the userspace conntrack is heating up.  That's a good
thing, but it means that we'll probably have some growing pains as it
relates to features and usages.  These are the topics and some
additional information that we came up with.

1. Making EST state match between linux kernel conntrack and userspace
   conntrack.  We will need to make sure that the windows datapath
   conntrack also matches.  The issue came down to the distinction about
   whether commit action should also imply that the connection is
   established.

2. Disabling conntrack helpers 'on by default' in the userspace
   datapath.  This represents possible security issue; users will want
   to disable helpers (or enable helpers) at their own discretion.
   One proposed resolution to this is simply disabling the helpers, and
   relying on the 'alg=' specifier in the conntrack action. Complicating
   this solution is existing users who rely on the existing solution -
   specifically those users with the tftp helper and pxe booting in an
   large data center.

3. Performance analysis deep-dive.  We'd like to get input on the
   performance of the userspace conntrack path.  There was discussion
   that the performance was suffering - anything here like additional
   tests people have, or data that can be shared is good.  The
   development community is interested in knowing what it means.

4. Hardware offload.  What will we need to present from that standpoint?
   Are there counters or other information that software will need to
   expose?  What does it mean for the userspace datapath to be aware of
   hardware offload for conntrack?

5. Additional protocol support and helpers.  We think SCTP, and SIP are
   important.  Any others?  Anyone think this is useful work to do?

Thanks all for the presentations and discussions.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [patch v1 3/4] conntrack: Disable algs by default.

[Aaron Conole]

Darrell Ball <db...@vmware.com> writes:

> On 11/20/17, 7:19 AM, "ovs-dev-boun...@openvswitch.org on behalf of
> Aaron Conole" <ovs-dev-boun...@openvswitch.org on behalf of
> acon...@redhat.com> wrote:
>
> Hi Darrell,
> 
> Darrell Ball <dlu...@gmail.com> writes:
> 
> > Presently, alg processing is enabled by default to exercise testing.
> > This is similar to kernels before 4.7.  The recommended default
> > behavior in the kernel is to only process algs if a helper is
> > supplied in a conntrack rule.  The behavior is changed to match the
> > later kernels.
> >
> > Signed-off-by: Darrell Ball <dlu...@gmail.com>
> > ---
> 
> I like having the alg processing off by default.  However, at this point
> some folks may be relying (or not care about) on the default behavior of
> the ct() action (which is to assume
> ALG=whatever-is-appropriate-to-the-port).  I think we probably will need
> to have a knob to enable / disable this.
> 
> This part has me worried.
> The kernel has taken the position that the user should operate such that a 
> helper is 
> needed to be specified. This gives the user total granular control as well.
>
> i.e. the kernel has taken the position that
> net.netfilter.nf_conntrack_helper should be off by default
> in all cases.
>
> Allowing the user to turn this on is a double-edged sword. Yes, it
> allows the user to not have to specify
> alg=blah, but is needing to specify alg= all that onerous?

I'm not sure.  How many scripts and orchestration tools need to be
rewritten to handle it?  Maybe it's early enough that no one will care.
I'm okay not having the knob, but if we get bugs, such a knob will have
to go in and probably need to be backported.  I'm simply trying to think
about that case.

> On the flip side, allowing the user to skip specifying “alg=blah”
> opens a security hole that will never be
> noticed until a problem occurs – that is really nasty. Given the
> kernel position, it may be ok to do the
> right thing here and not allow the user to shoot himself in the foot 
> accidently.
>
>
>
>
> More below.
> 
> >  lib/conntrack.c | 35 +--
> >  1 file changed, 29 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > index 7fbcfba..dea2fed 100644
> > --- a/lib/conntrack.c
> > +++ b/lib/conntrack.c
> > @@ -789,13 +789,34 @@ conn_clean(struct conntrack *ct, struct conn 
> *conn,
> >  }
> >  }
> >  
> > +static bool
> > +ct_verify_helper(const char *helper, enum ct_alg_ctl_type ct_alg_ctl)
> > +{
> 
> Since we know the algorithm at this point, doesn't it make sense to
> just associate the alg with the connection?  I say that because if I
> read this correctly (and I haven't had coffee yet - maybe I missed
> something) non-standard ports won't get their associations properly (so
> if I want to run ftp on port 2122 for example, the helper will not get
> associated).
> 
> If we take the approach I mentioned in 2/4, we can just associate the
> function pointer.  After all, the user asked for it - we should do what
> the user wants.
>
> It is orthogonal.
> This of CT_ALG_CTL_FTP and CT_ALG_CTL_TFTP as array indexes (I’ll
> explain this more in my response to patch 2).
> i.e. CT_ALG_CTL_FTP and CT_ALG_CTL_TFTP are not linked to specific L4 port 
> numbers.
>
>
> 
> > +if (ct_alg_ctl == CT_ALG_CTL_NONE) {
> > +return true;
> > +} else if (helper) {
> > +if ((ct_alg_ctl == CT_ALG_CTL_FTP) &&
> > + !strncmp(helper, "ftp", strlen("ftp"))) {
> > +return true;
> > +} else if ((ct_alg_ctl == CT_ALG_CTL_TFTP) &&
> > +   !strncmp(helper, "tftp", strlen("tftp"))) {
> > +return true;
> > +} else {
> > +return false;
> > +}
> > +} else {
> > +return false;
> > +}
> > +}
> > +
> >  /* This function is called with the bucket lock held. */
> >  static struct conn *
> >  conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
> > struct conn_lookup_ctx *ctx, bool commit, long long now,
> > const struct nat_action_info_t *nat_action_info,
> > struct conn *con

Re: [ovs-dev] [patch v1 4/4] conntrack: Change established state to match kernel.

[Aaron Conole]

Darrell Ball <db...@vmware.com> writes:

> On 11/20/17, 7:46 AM, "ovs-dev-boun...@openvswitch.org on behalf of
> Aaron Conole" <ovs-dev-boun...@openvswitch.org on behalf of
> acon...@redhat.com> wrote:
>
> Darrell Ball <dlu...@gmail.com> writes:
> 
> > Presently, the userpace connection tracker 'established' packet
> > state diverges from the kernel and this patch brings them in line.
> > The behavior is now that 'established' is only possible after a
> > reply packet is seen.
> > The previous behavior is hard to notice when rules are written to
> > commit a connection in the trusted direction, which is recommended.
> >
> > A test is added to verify this.
> >
> > The documentation is updated to describe the new behavior of
> > 'established' and also clarify 'new'.
> >
> > Signed-off-by: Darrell Ball <dlu...@gmail.com>
> > ---
> >  lib/conntrack-private.h |  1 +
> >  lib/conntrack.c | 21 -
> >  lib/meta-flow.xml   | 10 +++---
> >  tests/system-traffic.at | 35 +++
> >  4 files changed, 59 insertions(+), 8 deletions(-)
> >
> > diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> > index ac0198f..1f6a107 100644
> > --- a/lib/conntrack-private.h
> > +++ b/lib/conntrack-private.h
> > @@ -107,6 +107,7 @@ struct conn {
> >  uint8_t seq_skew_dir;
> >  /* True if alg data connection. */
> >  uint8_t alg_related;
> > +uint8_t reply_seen;
> 
> Curious - do you envision using this in the future in other areas of the
> code?  Just wondering why add it to the conn struct now.
>
> It is needed for this change; specifically, this check
>> +if ((*conn)->reply_seen) {
> The flag is added to keep context across different packets. For example, a 
> packet in
> the forward direction needs to know that a reply packet has been seen
> for that connection.
>
> I have patch to convert flags to a bitarray but that is for later and
> there are other considerations,
> so I deferred it.

I'm still confused, though.  In the code, it looks that having ctx->reply
will be good enough to set state to established.  This field is only
set in that condition.  That's why I ask.  I don't understand if there's
a plan to use this field in the future.

> >  };
> >  
> >  enum ct_update_res {
> > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > index dea2fed..323114a 100644
> > --- a/lib/conntrack.c
> > +++ b/lib/conntrack.c
> > @@ -928,6 +928,21 @@ nat_res_exhaustion:
> >  return NULL;
> >  }
> >  
> > +/* This function is called with the bucket lock held. */
> > +static void
> > +conn_handle_reply(struct dp_packet *pkt, const struct conn_lookup_ctx 
> *ctx,
> > +  struct conn **conn)
> > +{
> > +if (ctx->reply) {
> > +pkt->md.ct_state |= CS_REPLY_DIR;
> > +(*conn)->reply_seen = true;
> > +}
> > +if ((*conn)->reply_seen) {
> > + pkt->md.ct_state |= CS_ESTABLISHED;
> > + pkt->md.ct_state &= ~CS_NEW;
> > +}
> > +}
> > +
> >  static bool
> >  conn_update_state(struct conntrack *ct, struct dp_packet *pkt,
> >struct conn_lookup_ctx *ctx, struct conn **conn,
> > @@ -950,11 +965,7 @@ conn_update_state(struct conntrack *ct, struct 
> dp_packet *pkt,
> >  
> >  switch (res) {
> >  case CT_UPDATE_VALID:
> > -pkt->md.ct_state |= CS_ESTABLISHED;
> > -pkt->md.ct_state &= ~CS_NEW;
> > -if (ctx->reply) {
> > -pkt->md.ct_state |= CS_REPLY_DIR;
> > -}
> > +conn_handle_reply(pkt, ctx, conn);
> >  break;
> >  case CT_UPDATE_INVALID:
> >  pkt->md.ct_state = CS_INVALID;
> > diff --git a/lib/meta-flow.xml b/lib/meta-flow.xml
> > index 08ee0ec..33a2ad6 100644
> > --- a/lib/meta-flow.xml
> > +++ b/lib/meta-flow.xml
> > @@ -2493,13 +2493,17 @@ 
> actions=clone(load:0->NXM_OF_IN_PORT[],output:123)
> >
> >  new (0x01)
> >  
> > -  A new connection.  Set to 1 if this is a

Re: [ovs-dev] [PATCH] packets: Prefetch the packet metadata in cacheline1.

[Aaron Conole]

Bhanuprakash Bodireddy  writes:

> pkt_metadata_prefetch_init() is used to prefetch the packet metadata
> before initializing the metadata in pkt_metadata_init(). This is done
> for every packet in userspace datapath and is performance critical.
>
> Commit 99fc16c0 prefetches only cachline0 and cacheline2 as the metadata
> part of respective cachelines will be initialized by pkt_metadata_init().
>
> However in VXLAN case when popping the vxlan header, netdev_vxlan_pop_header()
> invokes pkt_metadata_init_tnl() which zeroes out metadata part of
> cacheline1 that wasn't prefetched earlier and causes performance
> degradation.
>
> By prefetching cacheline1, 9% performance improvement is observed.

Do we see a degredation in the non-vxlan case?  If not, then I don't see
any reason not to apply this patch.

> CC: Ben Pfaff 
> Fixes: 99fc16c0 ("Reorganize the pkt_metadata structure.")
> Signed-off-by: Bhanuprakash Bodireddy 
> ---
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [patch v1 4/4] conntrack: Change established state to match kernel.

[Aaron Conole]

Darrell Ball <db...@vmware.com> writes:

> On 11/20/17, 10:02 AM, "Aaron Conole" <acon...@redhat.com> wrote:
>
> Darrell Ball <db...@vmware.com> writes:
>     
> > On 11/20/17, 9:43 AM, "Aaron Conole" <acon...@redhat.com> wrote:
> >
> > Darrell Ball <db...@vmware.com> writes:
> > 
> > > On 11/20/17, 7:46 AM, "ovs-dev-boun...@openvswitch.org on behalf 
> of
> > > Aaron Conole" <ovs-dev-boun...@openvswitch.org on behalf of
> > > acon...@redhat.com> wrote:
> > >
> > > Darrell Ball <dlu...@gmail.com> writes:
> > > 
> > > > Presently, the userpace connection tracker 'established' 
> packet
> > > > state diverges from the kernel and this patch brings them 
> in line.
> > > > The behavior is now that 'established' is only possible 
> after a
> > > > reply packet is seen.
> > > > The previous behavior is hard to notice when rules are 
> written to
> > > > commit a connection in the trusted direction, which is 
> recommended.
> > > >
> > > > A test is added to verify this.
> > > >
> > > > The documentation is updated to describe the new behavior of
> > > > 'established' and also clarify 'new'.
> > > >
> > > > Signed-off-by: Darrell Ball <dlu...@gmail.com>
> > > > ---
> > > >  lib/conntrack-private.h |  1 +
> > > >  lib/conntrack.c | 21 -
> > > >  lib/meta-flow.xml   | 10 +++---
> > > >  tests/system-traffic.at | 35 
> +++
> > > >  4 files changed, 59 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/lib/conntrack-private.h 
> b/lib/conntrack-private.h
> > > > index ac0198f..1f6a107 100644
> > > > --- a/lib/conntrack-private.h
> > > > +++ b/lib/conntrack-private.h
> > > > @@ -107,6 +107,7 @@ struct conn {
> > > >  uint8_t seq_skew_dir;
> > > >  /* True if alg data connection. */
> > > >  uint8_t alg_related;
> > > > +uint8_t reply_seen;
> > > 
> > > Curious - do you envision using this in the future in
> > > other areas of the
> > > code?  Just wondering why add it to the conn struct now.
> > >
> > > It is needed for this change; specifically, this check
> > >> +if ((*conn)->reply_seen) {
> > > The flag is added to keep context across different packets. For
> > > example, a packet in
> > > the forward direction needs to know that a reply packet has been 
> seen
> > > for that connection.
> > >
> > > I have patch to convert flags to a bitarray but that is for later 
> and
> > > there are other considerations,
> > > so I deferred it.
> > 
> > I'm still confused, though.  In the code, it looks that having 
> ctx->reply
> > will be good enough to set state to established.  This field is only
> > set in that condition.  That's why I ask.  I don't understand if 
> there's
> > a plan to use this field in the future.
> >
> > Reply is per packet and not saved across packets.
> >
> 
> See comment at the block in question.
> 
> > 
> > > >  };
> > > >  
> > > >  enum ct_update_res {
> > > > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > > > index dea2fed..323114a 100644
> > > > --- a/lib/conntrack.c
> > > > +++ b/lib/conntrack.c
> > > > @@ -928,6 +928,21 @@ nat_res_exhaustion:
> > > >  return NULL;
> > > >  }
> > > >  
> > > > +/* This function is called with the bucket lock held. */
> > > > +static void
> > > > +conn_ha

Re: [ovs-dev] [patch v1 2/4] conntrack: Refactor algs.

[Aaron Conole]

Hi Darrell,

Darrell Ball  writes:

> Upcoming requirements for new algs make it necessary to split out
> alg helper more cleanly.
>
> Signed-off-by: Darrell Ball 
> ---

Thanks for jumping on this so quickly.

I think this and 3/4 should be an independent series.  They don't have
much to do with 1/4 or 4/4, and logically could go in (and be developed)
independently.

>  lib/conntrack.c | 73 
> ++---
>  1 file changed, 44 insertions(+), 29 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index b8d0e77..7fbcfba 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -67,6 +67,12 @@ enum ct_alg_mode {
>  CT_TFTP_MODE,
>  };
>  
> +enum ct_alg_ctl_type {
> +CT_ALG_CTL_NONE,
> +CT_ALG_CTL_FTP,
> +CT_ALG_CTL_TFTP,
> +};
> +

Any reason to hardcode these?  I think we could have a struct like:

struct ct_alg_helper {
const char *name;
bool (*process_dp_pkt)(struct conn *ct,
   const struct conn *expectation,
   const struct dp_packet *pkt,
   bool nat);
... /* not sure of what else would be needed, yet */;
};

Then we can register statically with some table.  I think it will
simplify the way conntrack helpers are added, as well as simplify the
code in the 'framework'/'core' area.

>  static bool conn_key_extract(struct conntrack *, struct dp_packet *,
>   ovs_be16 dl_type, struct conn_lookup_ctx *,
>   uint16_t zone);
> @@ -432,33 +438,47 @@ get_ip_proto(const struct dp_packet *pkt)
>  }
>  
>  static bool
> -is_ftp_ctl(const struct dp_packet *pkt)
> +is_ftp_ctl(const enum ct_alg_ctl_type ct_alg_ctl)
> +{
> +return ct_alg_ctl == CT_ALG_CTL_FTP;
> +}
> +
> +static enum ct_alg_ctl_type
> +get_alg_ctl_type(const struct dp_packet *pkt)
>  {
>  uint8_t ip_proto = get_ip_proto(pkt);
> +struct udp_header *uh = dp_packet_l4(pkt);
>  struct tcp_header *th = dp_packet_l4(pkt);
>  
> -/* CT_IPPORT_FTP is used because IPPORT_FTP in not defined in OSX,
> - * at least in in.h. Since this value will never change, just remove
> +/* CT_IPPORT_FTP/TFTP is used because IPPORT_FTP/TFTP in not defined
> + * in OSX, at least in in.h. Since these values will never change, remove
>   * the external dependency. */
> -#define CT_IPPORT_FTP 21
> +enum { CT_IPPORT_FTP = 21 };
> +enum { CT_IPPORT_TFTP = 69 };
>  
> -return (ip_proto == IPPROTO_TCP &&
> -(th->tcp_src == htons(CT_IPPORT_FTP) ||
> - th->tcp_dst == htons(CT_IPPORT_FTP)));
> +if (ip_proto == IPPROTO_UDP && uh->udp_dst == htons(CT_IPPORT_TFTP)) {
> +return CT_ALG_CTL_TFTP;
> +} else if (ip_proto == IPPROTO_TCP &&
> +   (th->tcp_src == htons(CT_IPPORT_FTP) ||
> +th->tcp_dst == htons(CT_IPPORT_FTP))) {
> +return CT_ALG_CTL_FTP;
> +}
> +return CT_ALG_CTL_NONE;
>  }
>  
> -static bool
> -is_tftp_ctl(const struct dp_packet *pkt)
> +static void
> +handle_alg_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx,
> +   struct dp_packet *pkt, enum ct_alg_ctl_type ct_alg_ctl,
> +   const struct conn *conn, long long now, bool nat,
> +   const struct conn *conn_for_expectation)
>  {
> - uint8_t ip_proto = get_ip_proto(pkt);
> - struct udp_header *uh = dp_packet_l4(pkt);
> -
> -/* CT_IPPORT_TFTP is used because IPPORT_TFTP in not defined in OSX,
> - * at least in in.h. Since this value will never change, remove
> - * the external dependency. */
> -#define CT_IPPORT_TFTP 69
> -return (ip_proto == IPPROTO_UDP &&
> -uh->udp_dst == htons(CT_IPPORT_TFTP));
> +/* ALG control packet handling with expectation creation. */
> +if (OVS_UNLIKELY((ct_alg_ctl == CT_ALG_CTL_FTP) && conn)) {
> +handle_ftp_ctl(ct, ctx, pkt, conn_for_expectation,
> +   now, CT_FTP_CTL_INTEREST, nat);
> +} else if (OVS_UNLIKELY((ct_alg_ctl == CT_ALG_CTL_TFTP) && conn)) {
> +handle_tftp_ctl(ct, conn_for_expectation, now);
> +}
>  }
>  
>  static void
> @@ -1110,10 +1130,11 @@ process_one(struct conntrack *ct, struct dp_packet 
> *pkt,
>  bool create_new_conn = false;
>  struct conn conn_for_un_nat_copy;
>  conn_for_un_nat_copy.conn_type = CT_CONN_TYPE_DEFAULT;
> -bool ftp_ctl = is_ftp_ctl(pkt);
> +
> +enum ct_alg_ctl_type ct_alg_ctl = get_alg_ctl_type(pkt);
>  
>  if (OVS_LIKELY(conn)) {
> -if (ftp_ctl) {
> +if (is_ftp_ctl(ct_alg_ctl)) {
>  /* Keep sequence tracking in sync with the source of the
>   * sequence skew. */
>  if (ctx->reply != conn->seq_skew_dir) {
> @@ -1173,9 +1194,8 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
>  set_label(pkt, conn, [0], [1]);
>  }
>  
> -

Re: [ovs-dev] [patch v1 3/4] conntrack: Disable algs by default.

[Aaron Conole]

Hi Darrell,

Darrell Ball  writes:

> Presently, alg processing is enabled by default to exercise testing.
> This is similar to kernels before 4.7.  The recommended default
> behavior in the kernel is to only process algs if a helper is
> supplied in a conntrack rule.  The behavior is changed to match the
> later kernels.
>
> Signed-off-by: Darrell Ball 
> ---

I like having the alg processing off by default.  However, at this point
some folks may be relying (or not care about) on the default behavior of
the ct() action (which is to assume
ALG=whatever-is-appropriate-to-the-port).  I think we probably will need
to have a knob to enable / disable this.

More below.

>  lib/conntrack.c | 35 +--
>  1 file changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 7fbcfba..dea2fed 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -789,13 +789,34 @@ conn_clean(struct conntrack *ct, struct conn *conn,
>  }
>  }
>  
> +static bool
> +ct_verify_helper(const char *helper, enum ct_alg_ctl_type ct_alg_ctl)
> +{

Since we know the algorithm at this point, doesn't it make sense to
just associate the alg with the connection?  I say that because if I
read this correctly (and I haven't had coffee yet - maybe I missed
something) non-standard ports won't get their associations properly (so
if I want to run ftp on port 2122 for example, the helper will not get
associated).

If we take the approach I mentioned in 2/4, we can just associate the
function pointer.  After all, the user asked for it - we should do what
the user wants.

> +if (ct_alg_ctl == CT_ALG_CTL_NONE) {
> +return true;
> +} else if (helper) {
> +if ((ct_alg_ctl == CT_ALG_CTL_FTP) &&
> + !strncmp(helper, "ftp", strlen("ftp"))) {
> +return true;
> +} else if ((ct_alg_ctl == CT_ALG_CTL_TFTP) &&
> +   !strncmp(helper, "tftp", strlen("tftp"))) {
> +return true;
> +} else {
> +return false;
> +}
> +} else {
> +return false;
> +}
> +}
> +
>  /* This function is called with the bucket lock held. */
>  static struct conn *
>  conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
> struct conn_lookup_ctx *ctx, bool commit, long long now,
> const struct nat_action_info_t *nat_action_info,
> struct conn *conn_for_un_nat_copy,
> -   const char *helper, const struct alg_exp_node *alg_exp)
> +   const char *helper, const struct alg_exp_node *alg_exp,
> +   enum ct_alg_ctl_type ct_alg_ctl)
>  {
>  struct conn *nc = NULL;
>  
> @@ -819,15 +840,16 @@ conn_not_found(struct conntrack *ct, struct dp_packet 
> *pkt,
>  return nc;
>  }
>  
> +if (!ct_verify_helper(helper, ct_alg_ctl)) {
> +return nc;
> +}
> +
>  unsigned bucket = hash_to_bucket(ctx->hash);
>  nc = new_conn(>buckets[bucket], pkt, >key, now);
>  ctx->conn = nc;
>  nc->rev_key = nc->key;
>  conn_key_reverse(>rev_key);
> -
> -if (helper) {
> -nc->alg = xstrdup(helper);
> -}
> +nc->alg = nullable_xstrdup(helper);
>  
>  if (alg_exp) {
>  nc->alg_related = true;
> @@ -1182,7 +1204,8 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
>  }
>  ct_rwlock_unlock(>resources_lock);
>  conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info,
> -  _for_un_nat_copy, helper, alg_exp);
> +  _for_un_nat_copy, helper, alg_exp,
> +  ct_alg_ctl);
>  }
>  write_ct_md(pkt, zone, conn, >key, alg_exp);
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev