Re: [ovs-dev] [PATCH] ofproto: Return error codes for Rule insertions

[Aravind Prasad]

> Currently, rule_insert() API does not have return value. There are some 
> possible
> scenarios where rule insertions can fail at run-time even though the static
> checks during rule_construct() had passed previously.
> Some possible scenarios for failure of rule insertions:
> **) Rule insertions can fail dynamically in Hybrid mode (both Openflow and
> Normal switch functioning coexist) where the CAM space could get suddenly
> filled up by Normal switch functioning and Openflow gets devoid of
> available space.
> **) Some deployments could have separate independent layers for HW rule
> insertions and application layer to interact with OVS. HW layer
> could face any dynamic issue during rule handling which application could
> not have predicted/captured in rule-construction phase.
> Rule-insert errors for bundles are handled too in this pull-request.
>
> Signed-off-by: Aravind Prasad S 

>Which switches does this help?

Hi Ben, These type of errors are possible in actual Hardware
implementations of OVS. It is possible that ofproto and netdev providers be
implemented for an actual HW/NPU. Usually, in such cases, in the rule
construct phase, all the static checks like verifying the qualifiers/
actions, CAM full checks could be done and the other related verifications.
But during the rule insert phase, it is possible that the rule insertion
may get failed in HW (runtime errors, HW errors and so on). Also, since HW
switches may support Hybrid mode (coexistence of Normal and Openflow
functioning), the possibility of this issue could be even more. Further,
when rule-insertion fails, it results in a stale state where the Controller
and Switch Flow-DB differ. Hence, we need a way to rollback for rule-insert
phase also. Kindly let me know your views. And sorry for re-sending the
review requests many times over. Will avoid in future.

On Fri, Jul 20, 2018 at 10:20 PM, Ben Pfaff  wrote:

On Thu, Jul 12, 2018 at 06:04:47PM +, Aravind Prasad S wrote:
> > Currently, rule_insert() API does not have return value. There are some
> possible
> > scenarios where rule insertions can fail at run-time even though the
> static
> > checks during rule_construct() had passed previously.
> > Some possible scenarios for failure of rule insertions:
> > **) Rule insertions can fail dynamically in Hybrid mode (both Openflow
> and
> > Normal switch functioning coexist) where the CAM space could get suddenly
> > filled up by Normal switch functioning and Openflow gets devoid of
> > available space.
> > **) Some deployments could have separate independent layers for HW rule
> > insertions and application layer to interact with OVS. HW layer
> > could face any dynamic issue during rule handling which application could
> > not have predicted/captured in rule-construction phase.
> > Rule-insert errors for bundles are handled too in this pull-request.
> >
> > Signed-off-by: Aravind Prasad S 
>
> Which switches does this help?
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ip_tunnel: Fix bugs that could crash kernel

[William Tu]

On Fri, Jul 20, 2018 at 11:04 AM, Yifeng Sun  wrote:
> Without this patch, OVS kernel module can delete itn->fb_tunnel_dev
> one more time than necessary, which causes kernel crash.
>
> On kernel 4.4.0-116-generic, the crash can be reproduced by running
> the simple test provided below through check-kernel.
>
>   make & make modules_install
>   rmmod ip_gre gre ip_tunnel
>   modprobe openvswitch
>   make check-kernel TESTSUITEFLAGS=x
>   dmesg
>
> Simple test:
>
> AT_SETUP([datapath - crash test])
> OVS_CHECK_GRE()
> ip link del gre0
> OVS_TRAFFIC_VSWITCHD_START()
> AT_CHECK([ovs-vsctl -- set bridge br0])
> ADD_BR([br-underlay], [set bridge br-underlay])
> AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
> AT_CHECK([ovs-ofctl add-flow br-underlay "actions=normal"])
> ADD_NAMESPACES(at_ns0)
> ADD_VETH(p0, at_ns0, br-underlay, "172.31.1.1/24")
> AT_CHECK([ip addr add dev br-underlay "172.31.1.100/24"])
> AT_CHECK([ip link set dev br-underlay up])
> ADD_OVS_TUNNEL([gre], [br0], [at_gre0], [172.31.1.1], [10.1.1.100/24])
> tcpdump -U -i br-underlay -w underlay.pcap &
> sleep 1
> OVS_TRAFFIC_VSWITCHD_STOP
> AT_CLEANUP
>
> Signed-off-by: Yifeng Sun 
> ---

LGTM, Thanks for the fix.
Acked-by: William Tu 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] ip_tunnel: Fix bugs that could crash kernel

[Yifeng Sun]

Without this patch, OVS kernel module can delete itn->fb_tunnel_dev
one more time than necessary, which causes kernel crash.

On kernel 4.4.0-116-generic, the crash can be reproduced by running
the simple test provided below through check-kernel.

  make & make modules_install
  rmmod ip_gre gre ip_tunnel
  modprobe openvswitch
  make check-kernel TESTSUITEFLAGS=x
  dmesg

Simple test:

AT_SETUP([datapath - crash test])  
OVS_CHECK_GRE()
ip link del gre0   
OVS_TRAFFIC_VSWITCHD_START()
AT_CHECK([ovs-vsctl -- set bridge br0])
ADD_BR([br-underlay], [set bridge br-underlay])
AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
AT_CHECK([ovs-ofctl add-flow br-underlay "actions=normal"])
ADD_NAMESPACES(at_ns0) 
ADD_VETH(p0, at_ns0, br-underlay, "172.31.1.1/24") 
AT_CHECK([ip addr add dev br-underlay "172.31.1.100/24"])  
AT_CHECK([ip link set dev br-underlay up]) 
ADD_OVS_TUNNEL([gre], [br0], [at_gre0], [172.31.1.1], [10.1.1.100/24]) 
tcpdump -U -i br-underlay -w underlay.pcap &   
sleep 1 
OVS_TRAFFIC_VSWITCHD_STOP  
AT_CLEANUP

Signed-off-by: Yifeng Sun 
---
 datapath/linux/compat/ip_tunnel.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/datapath/linux/compat/ip_tunnel.c 
b/datapath/linux/compat/ip_tunnel.c
index 54af1f178d36..d16e60fbfef0 100644
--- a/datapath/linux/compat/ip_tunnel.c
+++ b/datapath/linux/compat/ip_tunnel.c
@@ -482,8 +482,10 @@ void rpl_ip_tunnel_dellink(struct net_device *dev, struct 
list_head *head)
 
itn = net_generic(tunnel->net, tunnel->ip_tnl_net_id);
 
-   ip_tunnel_del(itn, netdev_priv(dev));
-unregister_netdevice_queue(dev, head);
+   if (itn->fb_tunnel_dev != dev) {
+   ip_tunnel_del(itn, netdev_priv(dev));
+   unregister_netdevice_queue(dev, head);
+   }
 }
 
 int rpl_ip_tunnel_init_net(struct net *net, int ip_tnl_net_id,
@@ -642,7 +644,8 @@ void rpl_ip_tunnel_uninit(struct net_device *dev)
struct ip_tunnel_net *itn;
 
itn = net_generic(net, tunnel->ip_tnl_net_id);
-   ip_tunnel_del(itn, netdev_priv(dev));
+   if (itn->fb_tunnel_dev != dev)
+   ip_tunnel_del(itn, netdev_priv(dev));
 }
 
 /* Do least required initialization, rest of init is done in tunnel_init call 
*/
-- 
2.7.4

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

Re: [ovs-dev] [PATCH] netdev-dpdk: Fix failure to configure flow control at netdev-init.

[Chandran, Sugesh]

Regards
_Sugesh


> -Original Message-
> From: Stokes, Ian
> Sent: Friday, July 20, 2018 8:41 PM
> To: Chandran, Sugesh ; ovs-
> d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] netdev-dpdk: Fix failure to configure flow 
> control
> at netdev-init.
> 
> On 7/20/2018 7:24 PM, Chandran, Sugesh wrote:
> > Hi Ian,
> >
> > Thank you for testing it on VF. Please find my comments below,
> >
> > Regards
> > _Sugesh
> >
> >> -Original Message-
> >> From: Stokes, Ian
> >> Sent: Friday, July 20, 2018 6:30 PM
> >> To: Chandran, Sugesh ; ovs-
> >> d...@openvswitch.org
> >> Subject: Re: [ovs-dev] [PATCH] netdev-dpdk: Fix failure to configure
> >> flow control at netdev-init.
> >>
> >> On 7/13/2018 6:13 PM, Ian Stokes wrote:
> >>> On 7/10/2018 2:23 PM, Sugesh Chandran wrote:
>  Configuring flow control at ixgbe netdev-init is throwing error in
>  port start.
> 
>  For eg: without this fix, user cannot configure flow control on
>  ixgbe dpdk port as below,
> 
>  "
>    ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk
>  \
>    options:dpdk-devargs=:05:00.1
>  options:rx-flow-ctrl=true "
> 
>  Instead,  it must be configured as two different commands,
> 
>  "
>    ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk
>  \
>       options:dpdk-devargs=:05:00.1
>    ovs-vsctl set Interface dpdk0 options:rx-flow-ctrl=true "
> 
>  The DPDK ixgbe driver is now validating all the 'rte_eth_fc_conf'
>  fields before
>  trying to configuring the dpdk ethdev. Hence OVS can no longer set
>  the 'dont care' fields to just '0' as before. This commit make sure
>  all the 'rte_eth_fc_conf' fields are populated with default values
>  before the dev init.
> 
> >>>
> >>> Thanks for the patch Sugesh, I've applied to dpdk_merge and it will
> >>> be part of this weeks pull request.
> >>>
> >>> I assume it should be backported to OVS 2.9 at least, do you know if
> >>> it should go to 2.8/2.7 also?
> >>
> >> Hi Sugesh,
> >>
> >> during further testing I found this breaks functionality for Virtual
> >> functions with OVS (specifically tested with igb_vf, i40e_vf and ixgbe_vf).
> >>
> >> The port itself will fail to initialize with the following error:
> >>
> >> netdev_dpdk|INFO|cannot get flow control parameters on port 2,
> >> err=-95
> >>
> >> And is not added. as such I think flow control should only be
> >> optional as there is no guarantee it will be available for a given
> >> dpdk device and if unavailable it should not stop the port from being
> initialized. > [Sugesh] I am not sure if we need to add this condition in OVS.
> > As a virtual switch, OVS should able to use these APIs irrespective of 
> > NIC/port
> types.
> > Actually we are masking this error , without the patch. It is possible
> > to hit this issue, when we configure the flow control on a VF port.
> 
> Ok, but now we have a situation that when adding a VF port, even without
> specifying the flow control option, it fails as now flow control parameters 
> are
> being passed on init irregardless of whether the user intends to use it or 
> not.
> 
> Now that I think of it I think this would probably break other port types as 
> well
> that don't support flow control (such as vdevs).
> 
> > Adding a condition in OVS to check a VF port for similar parameters
> > doesn’t looks like a clean approach? What do you think?
> 
> We've already had to do this for a few items (HW CRC enablement, LSC, MTU
> support). It's not clean but until DPDK provides a way of checking 
> capabilities
> before init it's the best we can do.
[Sugesh] We may need to feed this back to DPDK to see if we can get a standard 
way to know the capabilities.
The flow control is slightly different than other parameters that mentioned 
above.
It involved setting multiple parameters to enable . OVS doesn’t really configure
all of those parameters .Instead  It just only enable/disable the feature when 
user
is requested. Remaining parameters are left to its defaults(which is not zero).
So OVS must first query the flow control before trying to configure it.
Now the query will fail for unsupported hardware, and the error message will be 
reported as of now.
 AFAIK DPDK doesn’t have any way to check if a port can support flow 
control/not, so we cannot avoid the error.
But what we can do here is, Instead of calling the 'get' blindly,
limit it to call only when user trying to configure flow control in the system.
That will avoid the error in normal scenario.
Does it make sense?
I will send out the patch with those modifications.
> 
> >
> > This leads to another question , whats the impact of modifying other netdev
> parameters
> >   on a VF port. Does it error out/silently fail/work as normal netdev ports?
> 
> I think we can't fail completely based on an optional capability. And this is 
> not
> just for VF ports, conceivably you 

[ovs-dev] [PATCH v1] rhel: support kmod build against mulitple kernel versions, fedora

[Martin Xu]

This patch ports changes from kmod rhel6 spec file to fedora spec file,
to support packaging kernel modules built against multiple versions of
kernel sources.

RHEL 7.4 introduced backward incompatible changes in the kernel. As
a result, prebuilt PRM packages against kernels newer than 693.17.1
will cannot be used on systems with older kernels, vice versa.

Intended to work only on RHEL 7.4 (kernel version 3.10.0-693.yy.zz).
This patch allows multiple kernel version numbers delimited by
whitespace to be passed as variable "kversion". The result RPM packages
the kernel module .ko files from all specified kernel versions. For
example,

make rpm-fedora-kmod \
RPMBUILD_OPT='-D "kversion 3.10.0-693.1.1.el7.x86_64 \
3.10.0-693.17.1.el7.x86_64"'

By default, make tries to build against the current running kernel.

This patch also includes a script to update the weak-update symlinks
if the system kernel version is upgraded or downgraded after
openvswitch-kmod is installed.

Signed-off-by: Martin Xu 
CC: Greg Rose 
CC: Flavio Leitner 
---
 rhel/openvswitch-kmod-fedora.spec.in | 86 +++-
 1 file changed, 55 insertions(+), 31 deletions(-)

diff --git a/rhel/openvswitch-kmod-fedora.spec.in 
b/rhel/openvswitch-kmod-fedora.spec.in
index c0cd298..24f8290 100644
--- a/rhel/openvswitch-kmod-fedora.spec.in
+++ b/rhel/openvswitch-kmod-fedora.spec.in
@@ -1,6 +1,6 @@
 # Spec file for Open vSwitch.
 
-# Copyright (C) 2009, 2010, 2015 Nicira Networks, Inc.
+# Copyright (C) 2009, 2010, 2015, 2018 Nicira Networks, Inc.
 #
 # Copying and distribution of this file, with or without modification,
 # are permitted in any medium without royalty provided the copyright
@@ -26,6 +26,9 @@ Release: 1%{?dist}
 Source: openvswitch-%{version}.tar.gz
 #Source1: openvswitch-init
 Buildroot: /tmp/openvswitch-xen-rpm
+Provides: kmod-openvswitch
+Conflicts: kmod-openvswitch
+Obsoletes: kmod-openvswitch
 
 %description
 Open vSwitch provides standard network bridging functions augmented with
@@ -36,55 +39,76 @@ traffic. This package contains the kernel modules.
 %setup -q -n openvswitch-%{version}
 
 %build
-%configure --with-linux=/lib/modules/%{kernel}/build --enable-ssl
-make %{_smp_mflags} -C datapath/linux
+for kv in %{kversion}; do
+mkdir -p _$kv
+(cd _$kv && /bin/cp -f ../configure . && %configure --srcdir=.. \
+--with-linux=/usr/src/kernels/${kv}/ --enable-ssl)
+make %{_smp_mflags} -C _$kv/datapath/linux
+done
 
 %install
+export INSTALL_MOD_DIR=extra/openvswitch
 rm -rf $RPM_BUILD_ROOT
-make INSTALL_MOD_PATH=$RPM_BUILD_ROOT -C datapath/linux modules_install
+for kv in %{kversion}; do
+make INSTALL_MOD_PATH=$RPM_BUILD_ROOT -C _$kv/datapath/linux 
modules_install
+done
 mkdir -p $RPM_BUILD_ROOT/etc/depmod.d
-for module in $RPM_BUILD_ROOT/lib/modules/%{kernel}/extra/*.ko
-do
-modname="$(basename ${module})"
-echo "override ${modname%.ko} * extra" >> \
-$RPM_BUILD_ROOT/etc/depmod.d/kmod-openvswitch.conf
-echo "override ${modname%.ko} * weak-updates" >> \
-$RPM_BUILD_ROOT/etc/depmod.d/kmod-openvswitch.conf
+for kv in %{kversion}; do
+for module in $RPM_BUILD_ROOT/lib/modules/${kv}/extra/openvswitch/*.ko
+do
+modname="$(basename ${module})"
+grep -qsPo "^\s*override ${modname%.ko} \* extra\/openvwitch" \
+$RPM_BUILD_ROOT/etc/depmod.d/kmod-openvswitch.conf || \
+echo "override ${modname%.ko} * extra/openvswitch" >> \
+$RPM_BUILD_ROOT/etc/depmod.d/kmod-openvswitch.conf
+grep -qsPo "^\s*override ${modname%.ko} \* weak-updates\/openvwitch" \
+$RPM_BUILD_ROOT/etc/depmod.d/kmod-openvswitch.conf || \
+echo "override ${modname%.ko} * weak-updates/openvswitch" >> \
+$RPM_BUILD_ROOT/etc/depmod.d/kmod-openvswitch.conf
+done
 done
+install -d -m 0755 $RPM_BUILD_ROOT/usr/share/openvswitch/scripts
+install -p -m 0755 rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh \
+$RPM_BUILD_ROOT/usr/share/openvswitch/scripts/ovs-kmod-manage.sh
 
 %clean
 rm -rf $RPM_BUILD_ROOT
 
 %post
-# Ensure that modprobe will find our modules.
-for k in $(cd /lib/modules && /bin/ls); do
-[ -d "/lib/modules/$k/kernel/" ] && depmod -a "$k"
-done
-if [ -x "/sbin/weak-modules" ]; then
-for m in openvswitch vport-gre vport-stt vport-geneve \
- vport-lisp vport-vxlan; do
-echo "/lib/modules/%{kernel}/extra/$m.ko"
-done | /sbin/weak-modules --add-modules
-fi
-
-%postun
-for k in $(cd /lib/modules && /bin/ls); do
-[ -d "/lib/modules/$k/kernel/" ] && depmod -a "$k"
-done
-if [ "$1" = 0 ]; then  # Erase, not upgrade
+current_kernel=$(uname -r)
+IFS=. read installed_major installed_minor installed_micro installed_arch \
+installed_build <<<"${current_kernel##*-}"
+if [ "$installed_major" = "693" ]; then
+# Workaround for RHEL 7.4
+if [ -x "/usr/share/openvswitch/scripts/ovs-kmod-manage.sh" ]; then
+

Re: [ovs-dev] [PATCH] compat: Initialize IPv4 reassembly secret timer

[Gregory Rose]

On 7/20/2018 2:00 PM, Yi-Hung Wei wrote:

On Thu, Jul 19, 2018 at 6:48 PM, Greg Rose  wrote:

The RHEL 7 kernels expect the secret timer interval to be initialized
before calling the inet_frags_init() function.  By not initializing it
the inet_frags_secret_rebuild() function was running on every tick
rather than on the expected interval.  This caused occasional panics
from page faults when inet_frags_secret_rebuild() would try to rearm a
timer from the openvswitch kernel module which had just been removed.

Thanks Greg for the patch.  The bug hides much deeper than I would expect.


Also remove the prior, and now unnecessary, work around.

VMware BZ 2094203

A minor nit, maybe VMware-BZ: #2094203


I'm fine with that.  I wasn't aware of any specific format.




--- a/datapath/linux/compat/ip_fragment.c
+++ b/datapath/linux/compat/ip_fragment.c
@@ -812,6 +812,9 @@ int __init rpl_ipfrag_init(void)
  #ifdef HAVE_INET_FRAGS_WITH_FRAGS_WORK
 ip4_frags.frags_cache_name = ip_frag_cache_name;
  #endif
+#if RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8,0)
+   ip4_frags.secret_interval = 10 * 60 * HZ;
+#endif

This is a great catch!  Looks like we remove the secret_interval in
net-next commit
e3a57d18b061 ("inet: frag: remove periodic secret rebuild timer")
But somehow RHEL kernel still retain this field, and the
secret_interval was set to 10 * 60 * HZ.

I would recommend to set 'secret_interval' by grepping this filed in
'struct inet_frags' rather than by kernel version. So that if RHEL 8
kernel still have this field, your fix will apply.


That's a good point to raise.

My thinking was to specifically call it out for RHEL 7 because it hasn't 
been in the kernel since 3.16
and this bug is RHEL 7 specific. That's a long way back and I'm sure it 
will not be there in
the RHEL 8 kernel.  This makes it clear that it is only for RHEL 7 and 
nothing else.


However, if you and others feel strongly about it I don't mind making 
the change.





--- a/datapath/linux/compat/nf_conntrack_reasm.c
+++ b/datapath/linux/compat/nf_conntrack_reasm.c
@@ -643,6 +643,9 @@ int rpl_nf_ct_frag6_init(void)
  #ifdef HAVE_INET_FRAGS_WITH_FRAGS_WORK
 nf_frags.frags_cache_name = nf_frags_cache_name;
  #endif
+#if RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8,0)
+   nf_frags.secret_interval = 10 * 60 * HZ;
+#endif

Same for here.

-Yi-Hung


Thanks for the review!  I'll wait another day or two to get more feedback.

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

Re: [ovs-dev] [PATCH] compat: Initialize IPv4 reassembly secret timer

[Yi-Hung Wei]

On Thu, Jul 19, 2018 at 6:48 PM, Greg Rose  wrote:
> The RHEL 7 kernels expect the secret timer interval to be initialized
> before calling the inet_frags_init() function.  By not initializing it
> the inet_frags_secret_rebuild() function was running on every tick
> rather than on the expected interval.  This caused occasional panics
> from page faults when inet_frags_secret_rebuild() would try to rearm a
> timer from the openvswitch kernel module which had just been removed.
Thanks Greg for the patch.  The bug hides much deeper than I would expect.

> Also remove the prior, and now unnecessary, work around.
>
> VMware BZ 2094203
A minor nit, maybe VMware-BZ: #2094203

> --- a/datapath/linux/compat/ip_fragment.c
> +++ b/datapath/linux/compat/ip_fragment.c
> @@ -812,6 +812,9 @@ int __init rpl_ipfrag_init(void)
>  #ifdef HAVE_INET_FRAGS_WITH_FRAGS_WORK
> ip4_frags.frags_cache_name = ip_frag_cache_name;
>  #endif
> +#if RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8,0)
> +   ip4_frags.secret_interval = 10 * 60 * HZ;
> +#endif
This is a great catch!  Looks like we remove the secret_interval in
net-next commit
e3a57d18b061 ("inet: frag: remove periodic secret rebuild timer")
But somehow RHEL kernel still retain this field, and the
secret_interval was set to 10 * 60 * HZ.

I would recommend to set 'secret_interval' by grepping this filed in
'struct inet_frags' rather than by kernel version. So that if RHEL 8
kernel still have this field, your fix will apply.

> --- a/datapath/linux/compat/nf_conntrack_reasm.c
> +++ b/datapath/linux/compat/nf_conntrack_reasm.c
> @@ -643,6 +643,9 @@ int rpl_nf_ct_frag6_init(void)
>  #ifdef HAVE_INET_FRAGS_WITH_FRAGS_WORK
> nf_frags.frags_cache_name = nf_frags_cache_name;
>  #endif
> +#if RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8,0)
> +   nf_frags.secret_interval = 10 * 60 * HZ;
> +#endif
Same for here.

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

Re: [ovs-dev] [PATCH] netdev-dpdk: Fix failure to configure flow control at netdev-init.

[Ian Stokes]

On 7/20/2018 7:24 PM, Chandran, Sugesh wrote:

Hi Ian,

Thank you for testing it on VF. Please find my comments below,

Regards
_Sugesh


-Original Message-
From: Stokes, Ian
Sent: Friday, July 20, 2018 6:30 PM
To: Chandran, Sugesh ; ovs-
d...@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] netdev-dpdk: Fix failure to configure flow 
control
at netdev-init.

On 7/13/2018 6:13 PM, Ian Stokes wrote:

On 7/10/2018 2:23 PM, Sugesh Chandran wrote:

Configuring flow control at ixgbe netdev-init is throwing error in
port start.

For eg: without this fix, user cannot configure flow control on ixgbe
dpdk port as below,

"
  ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk \
  options:dpdk-devargs=:05:00.1 options:rx-flow-ctrl=true
"

Instead,  it must be configured as two different commands,

"
  ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk \
     options:dpdk-devargs=:05:00.1
  ovs-vsctl set Interface dpdk0 options:rx-flow-ctrl=true "

The DPDK ixgbe driver is now validating all the 'rte_eth_fc_conf'
fields before
trying to configuring the dpdk ethdev. Hence OVS can no longer set
the 'dont care' fields to just '0' as before. This commit make sure
all the 'rte_eth_fc_conf' fields are populated with default values
before the dev init.



Thanks for the patch Sugesh, I've applied to dpdk_merge and it will be
part of this weeks pull request.

I assume it should be backported to OVS 2.9 at least, do you know if
it should go to 2.8/2.7 also?


Hi Sugesh,

during further testing I found this breaks functionality for Virtual functions 
with
OVS (specifically tested with igb_vf, i40e_vf and ixgbe_vf).

The port itself will fail to initialize with the following error:

netdev_dpdk|INFO|cannot get flow control parameters on port 2, err=-95

And is not added. as such I think flow control should only be optional as there 
is
no guarantee it will be available for a given dpdk device and if unavailable it
should not stop the port from being initialized. > [Sugesh] I am not sure if we 
need to add this condition in OVS.

As a virtual switch, OVS should able to use these APIs irrespective of NIC/port 
types.
Actually we are masking this error , without the patch. It is possible to hit 
this issue,
when we configure the flow control on a VF port.


Ok, but now we have a situation that when adding a VF port, even without 
specifying the flow control option, it fails as now flow control 
parameters are being passed on init irregardless of whether the user 
intends to use it or not.


Now that I think of it I think this would probably break other port 
types as well that don't support flow control (such as vdevs).



Adding a condition in OVS to check a VF port for similar parameters doesn’t 
looks like
a clean approach? What do you think?


We've already had to do this for a few items (HW CRC enablement, LSC, 
MTU support). It's not clean but until DPDK provides a way of checking 
capabilities before init it's the best we can do.




This leads to another question , whats the impact of modifying other netdev 
parameters
  on a VF port. Does it error out/silently fail/work as normal netdev ports?


I think we can't fail completely based on an optional capability. And 
this is not just for VF ports, conceivably you can have HW ports that 
don't support a particular feature either supported by a netdev.


For the current patch you could warn a user that the feature is not 
supported and continue the initialization process (similar to the set 
mtu capability).


Ian





I've pulled this patch from the merge request for master, I think the branch
patches will have to be reworked also.

Ian


Ian

Signed-off-by: Sugesh Chandran 
---
   lib/netdev-dpdk.c | 15 +++
   1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
bb4d60f26..023b80d3e 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1065,14 +1065,6 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
   mbp_priv = rte_mempool_get_priv(dev->dpdk_mp->mp);
   dev->buf_size = mbp_priv->mbuf_data_room_size -
RTE_PKTMBUF_HEADROOM;
-
-    /* Get the Flow control configuration for DPDK-ETH */
-    diag = rte_eth_dev_flow_ctrl_get(dev->port_id, >fc_conf);
-    if (diag) {
-    VLOG_DBG("cannot get flow control parameters on port
"DPDK_PORT_ID_FMT
- ", err=%d", dev->port_id, diag);
-    }
-
   return 0;
   }
@@ -1773,6 +1765,13 @@ netdev_dpdk_set_config(struct netdev *netdev,
const struct smap *args,
   autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false);
   fc_mode = fc_mode_set[tx_fc_en][rx_fc_en];
+    /* Get the Flow control configuration for DPDK-ETH */
+    err = rte_eth_dev_flow_ctrl_get(dev->port_id, >fc_conf);
+    if (err) {
+    VLOG_INFO("cannot get flow control parameters on port
"DPDK_PORT_ID_FMT
+ ", err=%d", dev->port_id, err);
+    }
+
   if (dev->fc_conf.mode != 

[ovs-dev] [PATCH v5] ovn: Allow for automatic dynamic updates of IPAM

[Mark Michelson]

OVN offers a method of IP address management that allows for an IPv4 subnet or
IPv6 prefix to be specified on a logical switch. Then by specifying a
switch port's address as "dynamic" or " dynamic", OVN will
automatically assign addresses to the switch port.

While this works great for initial assignment of addresses, addresses do
not automatically adjust when changes are made to the switch's
configuration. For instance:
* If the subnet, ipv6_prefix, or exclude_ips for a logical switch
changes, the affected switch ports are not updated.
* If a switch port with a static IP address is added to the switch, and
that address conflicts with a dynamically assigned IP address, the
dynamic address is not updated.
* If a MAC address switched from being statically assigned to
dynamically assigned, the MAC address would not be updated.
* If a statically assigned MAC address changed, then the IPv6 address
would not be updated.

This patch solves all of the above issues by changing the algorithm for
IPAM assignment. There are essentially three steps.
1) While joining logical ports, all statically-assigned addresses (i.e.
any ports without "dynamic" addresses) have their addresses registered
to IPAM. This gives them top priority.
2) All logical ports with dynamic addresses are inspected. Any changes
that must be made to the addresses are collected to be made later. Any
addresses that do not require change are registered to IPAM. This allows
for previously assigned dynamic addresses to be kept.
3) All gathered changes are enacted.

The change contains new tests that ensure that dynamic addresses are
updated when appropriate.

This patch also alters some existing IPAM tests. Those tests assumed
that dynamic addresses would not be updated automatically, so those
tests either had to be altered or removed.

Signed-off-by: Mark Michelson 
Acked-by: Jakub Sitnicki 
---
v4->v5:
 Cleanups suggested by Jakub Sitnicki + rebase
 * Add some convenience pointers for shortened code.
 * Separate checking of updates of dynamic addresses and registration
   of unchanged addresses.
 * Use OVS_NOT_REACHED() instead of ovs_assert(0)

v3->v4:
 Print warning when multiple dynamic addresses are configured on a
 switch port. Ensure that dynamic addresses beyond the first on a switch
 port are ignored. Found by Ben Pfaff.

v2->v3:
 Fixed a checkpatch problem (line too long)

v1->v2:
 Rebased
---
 ovn/northd/ovn-northd.c | 396 +++-
 tests/ovn.at|  97 +---
 2 files changed, 361 insertions(+), 132 deletions(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 04a072ba8..572022897 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -985,9 +985,6 @@ ipam_add_port_addresses(struct ovn_datapath *od, struct 
ovn_port *op)
 for (size_t i = 0; i < op->nbsp->n_addresses; i++) {
 ipam_insert_lsp_addresses(od, op, op->nbsp->addresses[i]);
 }
-if (op->nbsp->dynamic_addresses) {
-ipam_insert_lsp_addresses(od, op, op->nbsp->dynamic_addresses);
-}
 } else if (op->nbrp) {
 struct lport_addresses lrp_networks;
 if (!extract_lrp_networks(op->nbrp, _networks)) {
@@ -1060,64 +1057,263 @@ ipam_get_unused_ip(struct ovn_datapath *od)
 return od->ipam_info.start_ipv4 + new_ip_index;
 }
 
+enum dynamic_update_type {
+NONE,/* No change to the address */
+REMOVE,  /* Address is no longer dynamic */
+STATIC,  /* Use static address (MAC only) */
+DYNAMIC, /* Assign a new dynamic address */
+};
+
+struct dynamic_address_update {
+struct ovs_list node;   /* In build_ipam()'s list of updates. */
+
+struct ovn_port *op;
+
+struct lport_addresses current_addresses;
+struct eth_addr static_mac;
+enum dynamic_update_type mac;
+enum dynamic_update_type ipv4;
+enum dynamic_update_type ipv6;
+};
+
+static enum dynamic_update_type
+dynamic_mac_changed(const char *lsp_addresses,
+struct dynamic_address_update *update)
+{
+   struct eth_addr ea;
+
+   if (ovs_scan(lsp_addresses, ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(ea))) {
+   if (eth_addr_equals(ea, update->current_addresses.ea)) {
+   return NONE;
+   } else {
+   /* MAC is still static, but it has changed */
+   update->static_mac = ea;
+   return STATIC;
+   }
+   }
+
+   uint64_t mac64 = eth_addr_to_uint64(update->current_addresses.ea);
+   if ((mac64 ^ MAC_ADDR_PREFIX) >> 24) {
+   return DYNAMIC;
+   } else {
+   return NONE;
+   }
+}
+
+static enum dynamic_update_type
+dynamic_ip4_changed(struct dynamic_address_update *update)
+{
+const struct ipam_info *ipam = >op->od->ipam_info;
+const struct lport_addresses *cur_addresses = >current_addresses;
+bool dynamic_ip4 = ipam->allocated_ipv4s != NULL;
+
+if (!dynamic_ip4) {
+if (update->current_addresses.n_ipv4_addrs) {
+return REMOVE;
+} 

Re: [ovs-dev] [PATCH] compat: Initialize IPv4 reassembly secret timer

[Gregory Rose]

On 7/19/2018 6:48 PM, Greg Rose wrote:

The RHEL 7 kernels expect the secret timer interval to be initialized
before calling the inet_frags_init() function.  By not initializing it
the inet_frags_secret_rebuild() function was running on every tick
rather than on the expected interval.  This caused occasional panics
from page faults when inet_frags_secret_rebuild() would try to rearm a
timer from the openvswitch kernel module which had just been removed.

Also remove the prior, and now unnecessary, work around.

VMware BZ 2094203

Fixes: 595e069a ("compat: Backport IPv4 reassembly.")
Signed-off-by: Greg Rose 


Copying Pravin and Flavio as well.

- Greg


---
  datapath/datapath.c| 10 --
  datapath/linux/compat/ip_fragment.c|  3 +++
  datapath/linux/compat/nf_conntrack_reasm.c |  3 +++
  3 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/datapath/datapath.c b/datapath/datapath.c
index 43f0d74..3ea240a 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -2478,16 +2478,6 @@ error:
  
  static void dp_cleanup(void)

  {
-#if RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8,0)
-   /* On RHEL 7.x kernels we hit a kernel paging error without
-* this barrier and subsequent hefty delay.  A process will
-* attempt to access openvwitch memory after it has been
-* unloaded.  Further debugging is needed on that but for
-* now let's not let customer machines panic.
-*/
-   rcu_barrier();
-   msleep(3000);
-#endif
dp_unregister_genl(ARRAY_SIZE(dp_genl_families));
ovs_netdev_exit();
unregister_netdevice_notifier(_dp_device_notifier);
diff --git a/datapath/linux/compat/ip_fragment.c 
b/datapath/linux/compat/ip_fragment.c
index 8f2012b..f910b99 100644
--- a/datapath/linux/compat/ip_fragment.c
+++ b/datapath/linux/compat/ip_fragment.c
@@ -812,6 +812,9 @@ int __init rpl_ipfrag_init(void)
  #ifdef HAVE_INET_FRAGS_WITH_FRAGS_WORK
ip4_frags.frags_cache_name = ip_frag_cache_name;
  #endif
+#if RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8,0)
+   ip4_frags.secret_interval = 10 * 60 * HZ;
+#endif
if (inet_frags_init(_frags)) {
pr_warn("IP: failed to allocate ip4_frags cache\n");
return -ENOMEM;
diff --git a/datapath/linux/compat/nf_conntrack_reasm.c 
b/datapath/linux/compat/nf_conntrack_reasm.c
index ea153c3..ce13112 100644
--- a/datapath/linux/compat/nf_conntrack_reasm.c
+++ b/datapath/linux/compat/nf_conntrack_reasm.c
@@ -643,6 +643,9 @@ int rpl_nf_ct_frag6_init(void)
  #ifdef HAVE_INET_FRAGS_WITH_FRAGS_WORK
nf_frags.frags_cache_name = nf_frags_cache_name;
  #endif
+#if RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8,0)
+   nf_frags.secret_interval = 10 * 60 * HZ;
+#endif
ret = inet_frags_init(_frags);
if (ret)
goto out;


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

Re: [ovs-dev] [PATCH net 0/2] openvswitch tests for nla_nest_start

[David Miller]

From: David Miller 
Date: Fri, 20 Jul 2018 12:15:36 -0700 (PDT)

> From: Stephen Hemminger 
> Date: Wed, 18 Jul 2018 09:12:14 -0700
> 
>> Coverity is looking for bugs here, and a couple of new bugzilla
>> reports showed up where nla_nest_start return is not checked
>> for NULL.
> 
> Series applied and patch #2 queued up for -stable, thanks Stephen.

Actually, this wasn't compile tested and fails to build. :-/

Please fix this and resubmit.

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

Re: [ovs-dev] [PATCH net 0/2] openvswitch tests for nla_nest_start

[David Miller]

From: Stephen Hemminger 
Date: Wed, 18 Jul 2018 09:12:14 -0700

> Coverity is looking for bugs here, and a couple of new bugzilla
> reports showed up where nla_nest_start return is not checked
> for NULL.

Series applied and patch #2 queued up for -stable, thanks Stephen.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ovs-dev, RFC] dp-packet: Don't resize DPBUF_DPDK packets.

[0-day Robot]

Bleep bloop.  Greetings Tiago Lam, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


build:
/bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I. 
   -I ./include -I ./include -I ./lib -I ./lib-Wstrict-prototypes -Wall 
-Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security 
-Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align 
-Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes 
-Wmissing-field-initializers -fno-strict-aliasing -Wshadow 
-Wno-null-pointer-arithmetic -Werror -Werror   -g -O2 -MT lib/dp-packet.lo -MD 
-MP -MF $depbase.Tpo -c -o lib/dp-packet.lo lib/dp-packet.c &&\
mv -f $depbase.Tpo $depbase.Plo
libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include 
-I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare 
-Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter 
-Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition 
-Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow 
-Wno-null-pointer-arithmetic -Werror -Werror -g -O2 -MT lib/dp-packet.lo -MD 
-MP -MF lib/.deps/dp-packet.Tpo -c lib/dp-packet.c -o lib/dp-packet.o
lib/dp-packet.c: In function 'dp_packet_is_tailroom_avail':
lib/dp-packet.c:280:47: error: unused parameter 'b' [-Werror=unused-parameter]
 dp_packet_is_tailroom_avail(struct dp_packet *b, size_t size)
   ^
lib/dp-packet.c:280:57: error: unused parameter 'size' 
[-Werror=unused-parameter]
 dp_packet_is_tailroom_avail(struct dp_packet *b, size_t size)
 ^
lib/dp-packet.c: In function 'dp_packet_is_headroom_avail':
lib/dp-packet.c:309:47: error: unused parameter 'b' [-Werror=unused-parameter]
 dp_packet_is_headroom_avail(struct dp_packet *b, size_t size)
   ^
lib/dp-packet.c:309:57: error: unused parameter 'size' 
[-Werror=unused-parameter]
 dp_packet_is_headroom_avail(struct dp_packet *b, size_t size)
 ^
lib/dp-packet.c: At top level:
cc1: error: unrecognized command line option "-Wno-null-pointer-arithmetic" 
[-Werror]
cc1: all warnings being treated as errors
make[2]: *** [lib/dp-packet.lo] Error 1
make[2]: Leaving directory 
`/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory 
`/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make: *** [all] Error 2


Please check this out.  If you feel there has been an error, please email 
acon...@bytheb.org

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] netdev-dpdk: Fix failure to configure flow control at netdev-init.

[Chandran, Sugesh]

Hi Ian,

Thank you for testing it on VF. Please find my comments below,

Regards
_Sugesh

> -Original Message-
> From: Stokes, Ian
> Sent: Friday, July 20, 2018 6:30 PM
> To: Chandran, Sugesh ; ovs-
> d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] netdev-dpdk: Fix failure to configure flow 
> control
> at netdev-init.
> 
> On 7/13/2018 6:13 PM, Ian Stokes wrote:
> > On 7/10/2018 2:23 PM, Sugesh Chandran wrote:
> >> Configuring flow control at ixgbe netdev-init is throwing error in
> >> port start.
> >>
> >> For eg: without this fix, user cannot configure flow control on ixgbe
> >> dpdk port as below,
> >>
> >> "
> >>  ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk \
> >>  options:dpdk-devargs=:05:00.1 options:rx-flow-ctrl=true
> >> "
> >>
> >> Instead,  it must be configured as two different commands,
> >>
> >> "
> >>  ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk \
> >>     options:dpdk-devargs=:05:00.1
> >>  ovs-vsctl set Interface dpdk0 options:rx-flow-ctrl=true "
> >>
> >> The DPDK ixgbe driver is now validating all the 'rte_eth_fc_conf'
> >> fields before
> >> trying to configuring the dpdk ethdev. Hence OVS can no longer set
> >> the 'dont care' fields to just '0' as before. This commit make sure
> >> all the 'rte_eth_fc_conf' fields are populated with default values
> >> before the dev init.
> >>
> >
> > Thanks for the patch Sugesh, I've applied to dpdk_merge and it will be
> > part of this weeks pull request.
> >
> > I assume it should be backported to OVS 2.9 at least, do you know if
> > it should go to 2.8/2.7 also?
> 
> Hi Sugesh,
> 
> during further testing I found this breaks functionality for Virtual 
> functions with
> OVS (specifically tested with igb_vf, i40e_vf and ixgbe_vf).
> 
> The port itself will fail to initialize with the following error:
> 
> netdev_dpdk|INFO|cannot get flow control parameters on port 2, err=-95
> 
> And is not added. as such I think flow control should only be optional as 
> there is
> no guarantee it will be available for a given dpdk device and if unavailable 
> it
> should not stop the port from being initialized.
[Sugesh] I am not sure if we need to add this condition in OVS. 
As a virtual switch, OVS should able to use these APIs irrespective of NIC/port 
types.
Actually we are masking this error , without the patch. It is possible to hit 
this issue,
when we configure the flow control on a VF port.
Adding a condition in OVS to check a VF port for similar parameters doesn’t 
looks like
a clean approach? What do you think?

This leads to another question , whats the impact of modifying other netdev 
parameters
 on a VF port. Does it error out/silently fail/work as normal netdev ports?


> 
> I've pulled this patch from the merge request for master, I think the branch
> patches will have to be reworked also.
> 
> Ian
> >
> > Ian
> >> Signed-off-by: Sugesh Chandran 
> >> ---
> >>   lib/netdev-dpdk.c | 15 +++
> >>   1 file changed, 7 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> >> bb4d60f26..023b80d3e 100644
> >> --- a/lib/netdev-dpdk.c
> >> +++ b/lib/netdev-dpdk.c
> >> @@ -1065,14 +1065,6 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
> >>   mbp_priv = rte_mempool_get_priv(dev->dpdk_mp->mp);
> >>   dev->buf_size = mbp_priv->mbuf_data_room_size -
> >> RTE_PKTMBUF_HEADROOM;
> >> -
> >> -    /* Get the Flow control configuration for DPDK-ETH */
> >> -    diag = rte_eth_dev_flow_ctrl_get(dev->port_id, >fc_conf);
> >> -    if (diag) {
> >> -    VLOG_DBG("cannot get flow control parameters on port
> >> "DPDK_PORT_ID_FMT
> >> - ", err=%d", dev->port_id, diag);
> >> -    }
> >> -
> >>   return 0;
> >>   }
> >> @@ -1773,6 +1765,13 @@ netdev_dpdk_set_config(struct netdev *netdev,
> >> const struct smap *args,
> >>   autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false);
> >>   fc_mode = fc_mode_set[tx_fc_en][rx_fc_en];
> >> +    /* Get the Flow control configuration for DPDK-ETH */
> >> +    err = rte_eth_dev_flow_ctrl_get(dev->port_id, >fc_conf);
> >> +    if (err) {
> >> +    VLOG_INFO("cannot get flow control parameters on port
> >> "DPDK_PORT_ID_FMT
> >> + ", err=%d", dev->port_id, err);
> >> +    }
> >> +
> >>   if (dev->fc_conf.mode != fc_mode || autoneg !=
> >> dev->fc_conf.autoneg) {
> >>   dev->fc_conf.mode = fc_mode;
> >>   dev->fc_conf.autoneg = autoneg;
> >>
> >
> > ___
> > 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

[ovs-dev] [RFC] dp-packet: Don't resize DPBUF_DPDK packets.

[Tiago Lam]

Up until now DPDK packets (DPBUF_DPDK) could call dp_packet_resize__()
without any constraint, leading to call OVS_NOT_REACHED() and premature
termination of the OvS process if one single packet didn't have enough
tailroom or headroom space for a specific operation, such as pushing or
popping a header.

To fix this, two functions are introduced, dp_packet_is_tailroom_avail()
and dp_packet_is_headroom_avail(), which are now used to check if
there's enough tailroom or headroom space, respectively, before
proceeding with calling the dp_packet_prealloc_tailroom() and
dp_packet_prealloc_headroom() functions, thus avoiding the call to
dp_packet_resize__() and OVS_NOT_REACHED() for DPDK packets.

Since put_uninit() and push_uninit() may now return NULL for DPDK
packets, the places where these operations may be called from a DPDK
packet now check for NULL and log a message where appropriate. For
example, eth_push_vlan() in packets.c, now returns 1 if the
push_uninit() operation on the packet fails. Later on, the caller,
odp_execute_actions(), drops the packet upon checking that the return is
different from 0 (error).

CC: Anju Thomas 
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-dev/2018-May/346649.html
Signed-off-by: Tiago Lam 
---
 lib/dp-packet.c | 126 +---
 lib/netdev-native-tnl.c |  24 +++--
 lib/netdev-native-tnl.h |   6 +--
 lib/netdev-provider.h   |   2 +-
 lib/netdev.c|  16 --
 lib/odp-execute.c   |  49 +++
 lib/packets.c   |  38 +--
 lib/packets.h   |   8 +--
 8 files changed, 224 insertions(+), 45 deletions(-)

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 443c225..058b653 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -239,6 +239,9 @@ dp_packet_resize__(struct dp_packet *b, size_t 
new_headroom, size_t new_tailroom
 
 switch (b->source) {
 case DPBUF_DPDK:
+/* DPDK mbufs have a fixed layout, meaning they can't be resized per
+ * say. As a result, this operation shouldn't be called for DPBUF_DPDK
+ * packets */
 OVS_NOT_REACHED();
 
 case DPBUF_MALLOC:
@@ -273,9 +276,27 @@ dp_packet_resize__(struct dp_packet *b, size_t 
new_headroom, size_t new_tailroom
 }
 }
 
-/* Ensures that 'b' has room for at least 'size' bytes at its tail end,
- * reallocating and copying its data if necessary.  Its headroom, if any, is
- * preserved. */
+static bool
+dp_packet_is_tailroom_avail(struct dp_packet *b, size_t size)
+{
+#ifdef DPDK_NETDEV
+if (b->source == DPBUF_DPDK) {
+if (size > dp_packet_tailroom(b)) {
+return false;
+}
+
+return true;
+}
+#endif
+return true;
+}
+
+/* For non-DPDK packets, ensures that 'b' has room for at least 'size' bytes at
+ * its tail end, reallocating and copying its data if necessary. Its headroom,
+ * if any, is preserved.
+ *
+ * For DPDK packets, no reallocation is performed. The caller is responsible
+ * for ensuring there's enough tailroom space available. */
 void
 dp_packet_prealloc_tailroom(struct dp_packet *b, size_t size)
 {
@@ -284,9 +305,27 @@ dp_packet_prealloc_tailroom(struct dp_packet *b, size_t 
size)
 }
 }
 
-/* Ensures that 'b' has room for at least 'size' bytes at its head,
- * reallocating and copying its data if necessary.  Its tailroom, if any, is
- * preserved. */
+static bool
+dp_packet_is_headroom_avail(struct dp_packet *b, size_t size)
+{
+#ifdef DPDK_NETDEV
+if (b->source == DPBUF_DPDK) {
+if (size > dp_packet_headroom(b)) {
+return false;
+}
+
+return true;
+}
+#endif
+return true;
+}
+
+/* For non-DPDK packets, ensures that 'b' has room for at least 'size' bytes at
+ * its head, reallocating and copying its data if necessary.  Its tailroom, if
+ * any, is preserved.
+ *
+ * For DPDK packets, no reallocation is performed. The caller is responsible
+ * for ensuring there's enough headroom space available. */
 void
 dp_packet_prealloc_headroom(struct dp_packet *b, size_t size)
 {
@@ -315,11 +354,19 @@ dp_packet_shift(struct dp_packet *b, int delta)
 
 /* Appends 'size' bytes of data to the tail end of 'b', reallocating and
  * copying its data if necessary.  Returns a pointer to the first byte of the
- * new data, which is left uninitialized. */
+ * new data, which is left uninitialized.
+ *
+ * For DPDK packets, no reallocation is performed and NULL may be returned if
+ * 'size' is bigger than the available tailroom space. */
 void *
 dp_packet_put_uninit(struct dp_packet *b, size_t size)
 {
 void *p;
+
+if (!dp_packet_is_tailroom_avail(b, size)) {
+return NULL;
+}
+
 dp_packet_prealloc_tailroom(b, size);
 p = dp_packet_tail(b);
 dp_packet_set_size(b, dp_packet_size(b) + size);
@@ -328,22 +375,34 @@ dp_packet_put_uninit(struct dp_packet *b, size_t size)
 
 /* Appends 'size' zeroed bytes to the tail end of 'b'.  Data in 'b' is
  * reallocated 

[ovs-dev] [RFC] Don't resize DPBUF_DPDK packets.

[Tiago Lam]

This RFC is a proposal to avoid the manual assert in dp_packet_resize__() for
DPBUF_DPDK packets. It has been reported before, in [1], as something that can
lead to crashes.

Overall I can see two approaches to this problem:
1. Modify dp_packet's API entirely to deal with errors and report them to
   callers appropriately;
2. For DPDK packets only, check a priori if the prealloc_tailroom() and
   prealloc_headroom() operations are possible or not (the only callers of
   dp_packet_resize__(), which leads to the manual assert).

Approach 1. would lead to a fair amount of change without giving much in return
when using non-DPDK packets (as DPBUF_MALLOC). For example, any packet created
with dp_packet_new() / dp_packet_init() / dp_packet_use_stub(), or even cloned
(a DPBUF_DPDK is cloned into a DPBUF_MALLOC, for example), wouldn't have the
mentioned limitations.

Instead, this approach follows 2. and modifies dp-packet.c so that
dp_packet_put_uninit() and dp_packet_push_uninit() don't call neither
dp_packet_prealloc_tailroom() nor dp_packet_prealloc_headroom() for DPBUF_DPDK
packets. Instead, they check if there's enough space beforehand, and if not
return NULL, instead of the normal pointer to the data. Otherwise the
operations would continue as expected.

This means we would need to deal with the NULL case only where we may be
using DPBUF_DPDK packets (all the other cases remain unaffected as NULL
won't be possible there). The cases I've identified it happens is where
headers are being pushed (which seems to be what's causing the crashes
in [2] as well):
- In eth_push_vlan(), push_eth(), push_mpls() and push_nsh(), which get
  called from odp_execute_actions();
- In netdev_tnl_push_ip_header(), which gets called by the native
  tunnels implementations, such as netdev_gre_push_header() for GRE, that
  is ultimately called by push_tnl_action().

I haven't found a case where dp_packet_put_uninit() is actually used in
a DPBUF_DPDK packet. In all cases it seems to be a result of a packet
created locally by using dp_packet_new() or some variant.

The system-userspace-testsuite tests have been run successfully with theses
changes, but no other tests have been performed.

[1] https://mail.openvswitch.org/pipermail/ovs-dev/2018-May/346649.html

Tiago Lam (1):
  dp-packet: Don't resize DPBUF_DPDK packets.

 lib/dp-packet.c | 126 +---
 lib/netdev-native-tnl.c |  24 +++--
 lib/netdev-native-tnl.h |   6 +--
 lib/netdev-provider.h   |   2 +-
 lib/netdev.c|  16 --
 lib/odp-execute.c   |  49 +++
 lib/packets.c   |  38 +--
 lib/packets.h   |   8 +--
 8 files changed, 224 insertions(+), 45 deletions(-)

-- 
2.7.4

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

Re: [ovs-dev] Revert "dp-packet: Handle multi-seg mbufs in resize__()."

[0-day Robot]

Bleep bloop.  Greetings Tiago Lam, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


git-am:
fatal: sha1 information is lacking or useless (lib/dp-packet.c).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Revert "dp-packet: Handle multi-seg mbufs in resize__()."
The copy of the patch that failed is found in:
   
/var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch
When you have resolved this problem, run "git am --resolved".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Please check this out.  If you feel there has been an error, please email 
acon...@bytheb.org

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] netdev-dpdk: Fix failure to configure flow control at netdev-init.

[Ian Stokes]

On 7/13/2018 6:13 PM, Ian Stokes wrote:

On 7/10/2018 2:23 PM, Sugesh Chandran wrote:

Configuring flow control at ixgbe netdev-init is throwing error in port
start.

For eg: without this fix, user cannot configure flow control on ixgbe 
dpdk

port as below,

"
 ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk \
 options:dpdk-devargs=:05:00.1 options:rx-flow-ctrl=true
"

Instead,  it must be configured as two different commands,

"
 ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk \
    options:dpdk-devargs=:05:00.1
 ovs-vsctl set Interface dpdk0 options:rx-flow-ctrl=true
"

The DPDK ixgbe driver is now validating all the 'rte_eth_fc_conf' 
fields before

trying to configuring the dpdk ethdev. Hence OVS can no longer set the
'dont care' fields to just '0' as before. This commit make sure all the
'rte_eth_fc_conf' fields are populated with default values before the dev
init.



Thanks for the patch Sugesh, I've applied to dpdk_merge and it will be 
part of this weeks pull request.


I assume it should be backported to OVS 2.9 at least, do you know if it 
should go to 2.8/2.7 also?


Hi Sugesh,

during further testing I found this breaks functionality for Virtual 
functions with OVS (specifically tested with igb_vf, i40e_vf and ixgbe_vf).


The port itself will fail to initialize with the following error:

netdev_dpdk|INFO|cannot get flow control parameters on port 2, err=-95

And is not added. as such I think flow control should only be optional 
as there is no guarantee it will be available for a given dpdk device 
and if unavailable it should not stop the port from being initialized.


I've pulled this patch from the merge request for master, I think the 
branch patches will have to be reworked also.


Ian


Ian

Signed-off-by: Sugesh Chandran 
---
  lib/netdev-dpdk.c | 15 +++
  1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index bb4d60f26..023b80d3e 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1065,14 +1065,6 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
  mbp_priv = rte_mempool_get_priv(dev->dpdk_mp->mp);
  dev->buf_size = mbp_priv->mbuf_data_room_size - 
RTE_PKTMBUF_HEADROOM;

-
-    /* Get the Flow control configuration for DPDK-ETH */
-    diag = rte_eth_dev_flow_ctrl_get(dev->port_id, >fc_conf);
-    if (diag) {
-    VLOG_DBG("cannot get flow control parameters on port 
"DPDK_PORT_ID_FMT

- ", err=%d", dev->port_id, diag);
-    }
-
  return 0;
  }
@@ -1773,6 +1765,13 @@ netdev_dpdk_set_config(struct netdev *netdev, 
const struct smap *args,

  autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false);
  fc_mode = fc_mode_set[tx_fc_en][rx_fc_en];
+    /* Get the Flow control configuration for DPDK-ETH */
+    err = rte_eth_dev_flow_ctrl_get(dev->port_id, >fc_conf);
+    if (err) {
+    VLOG_INFO("cannot get flow control parameters on port 
"DPDK_PORT_ID_FMT

+ ", err=%d", dev->port_id, err);
+    }
+
  if (dev->fc_conf.mode != fc_mode || autoneg != 
dev->fc_conf.autoneg) {

  dev->fc_conf.mode = fc_mode;
  dev->fc_conf.autoneg = autoneg;



___
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

[ovs-dev] [PATCH] Revert "dp-packet: Handle multi-seg mbufs in resize__()."

[Tiago Lam]

This reverts commit bc4b614. The commit tries to alleviate the call to
OVS_NOT_REACHED() in dp_packet_resize__(), for DPDK packets, by trying
to reuse the available tailroom space when no more headroom space is
available, and vice-versa. A simpler approach is to mitigate the call to
dp_packet_resize__() first, when DPDK packets are in use. Later, if
needed, this approach can be revisited.

Additionally, it also fixes the tests that were relying on the removed
functionality.

CC: Darrell Ball 
Signed-off-by: Tiago Lam 
---
Note that the above commit, bc4b614, is in dpdk_merge still, and not yet merged 
to master.
---
 lib/dp-packet.c | 48 ++--
 tests/test-dpdk-mbufs.c | 27 +++
 2 files changed, 13 insertions(+), 62 deletions(-)

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index ae060e2..6773535 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -285,51 +285,9 @@ dp_packet_resize__(struct dp_packet *b, size_t 
new_headroom, size_t new_tailroom
 new_allocated = new_headroom + dp_packet_size(b) + new_tailroom;
 
 switch (b->source) {
-/* When resizing mbufs, both a single mbuf and multi-segment mbufs (where
- * data is not contigously held in memory), both the headroom and the
- * tailroom available will be used to make more space for where data needs
- * to be inserted. I.e if there's not enough headroom, data may be shifted
- * right if there's enough tailroom.
- * However, this is not bulletproof and in some cases the space available
- * won't be enough - in those cases, an error should be returned and the
- * packet dropped. */
 case DPBUF_DPDK:
-{
-size_t miss_len;
-
-if (new_headroom == dp_packet_headroom(b)) {
-/* This is a tailroom adjustment. Since there's no tailroom space
- * left, try and shift data towards the head to free up tail space,
- * if there's enough headroom */
-
-miss_len = new_tailroom - dp_packet_tailroom(b);
-
-if (miss_len <= new_headroom) {
-dp_packet_shift(b, -miss_len);
-} else {
-/* XXX: Handle error case and report error to caller */
-OVS_NOT_REACHED();
-}
-} else {
-/* Otherwise, this is a headroom adjustment. Try to shift data
- * towards the tail to free up head space, if there's enough
- * tailroom */
-
-miss_len = new_headroom - dp_packet_headroom(b);
-
-
-if (miss_len <= new_tailroom) {
-dp_packet_shift(b, miss_len);
-} else {
-/* XXX: Handle error case and report error to caller */
-OVS_NOT_REACHED();
-}
-}
-
-new_base = dp_packet_base(b);
+OVS_NOT_REACHED();
 
-break;
-}
 case DPBUF_MALLOC:
 if (new_headroom == dp_packet_headroom(b)) {
 new_base = xrealloc(dp_packet_base(b), new_allocated);
@@ -353,9 +311,7 @@ dp_packet_resize__(struct dp_packet *b, size_t 
new_headroom, size_t new_tailroom
 OVS_NOT_REACHED();
 }
 
-if (b->source != DPBUF_DPDK) {
-dp_packet_set_allocated(b, new_allocated);
-}
+dp_packet_set_allocated(b, new_allocated);
 dp_packet_set_base(b, new_base);
 
 new_data = (char *) new_base + new_headroom;
diff --git a/tests/test-dpdk-mbufs.c b/tests/test-dpdk-mbufs.c
index 1c77038..8168cae 100644
--- a/tests/test-dpdk-mbufs.c
+++ b/tests/test-dpdk-mbufs.c
@@ -215,15 +215,6 @@ dpdk_pkt_put(struct dp_packet *pkt, void *p, size_t size) {
 
 dp_packet_mbuf_write(fmbuf, 0, size, p);
 
-/* Adjust size of intermediate mbufs from current tail to end */
-/*size_t pkt_len = size;
-while (fmbuf && pkt_len > 0) {
-fmbuf->data_len = MIN(pkt_len, fmbuf->buf_len - fmbuf->data_off);
-pkt_len -= fmbuf->data_len;
-
-fmbuf = fmbuf->next;
-}*/
-
 dp_packet_set_size(pkt, size);
 
 return pkt;
@@ -256,17 +247,21 @@ test_dpdk_packet_insert_tailroom_and_headroom(void) {
 struct dp_packet *pkt = dpdk_mp_alloc_pkt(mp);
 ovs_assert(pkt != NULL);
 
+/* Reserve 256B of header */
+size_t head_len = 256;
+dp_packet_reserve(pkt, head_len);
+
 /* Put the first 512B of "test_str" in the packet's header */
 size_t str_len = 512;
 char *p = dp_packet_put(pkt, test_str, str_len);
 ovs_assert(p != NULL);
-/* Allocate extra 256B of header */
-size_t head_len = 256;
+
+/* Fill the reserved 256B of header */
 p = dp_packet_push_uninit(pkt, head_len);
 ovs_assert(p != NULL);
 
 /* Check properties and data are as expected */
-assert_single_mbuf(pkt, 0, str_len + head_len);
+assert_single_mbuf(pkt, RTE_PKTMBUF_HEADROOM, str_len + head_len);
 
 /* Check the data inserted in the packet is correct */
 char data[str_len + head_len];
@@ -296,8 +291,8 @@ 

Re: [ovs-dev] [PATCH v5 08/14] dp-packet: Handle multi-seg mbufs in resize__().

[Lam, Tiago]

On 19/07/2018 00:02, Darrell Ball wrote:
> 
> 
> On Tue, Jul 17, 2018 at 12:59 AM, Lam, Tiago  > wrote:
> 
> On 16/07/2018 09:37, Lam, Tiago wrote:
> > On 13/07/2018 18:54, Darrell Ball wrote:
> >> Thanks for the patch.
> >>
> >> A few queries inline.
> >>
> >
> > Hi Darrell,
> >
> > Thanks for your inputs. I've replied in-line as well.
> >
> >> On Wed, Jul 11, 2018 at 11:23 AM, Tiago Lam  
> >> >> wrote:
> >>
> >>     When enabled with DPDK OvS relies on mbufs allocated by
> mempools to
> >>     receive and output data on DPDK ports. Until now, each OvS
> dp_packet has
> >>     had only one mbuf associated, which is allocated with the maximum
> >>     possible size, taking the MTU into account. This approach,
> however,
> >>     doesn't allow us to increase the allocated size in an mbuf,
> if needed,
> >>     since an mbuf is allocated and initialised upon mempool
> creation. Thus,
> >>     in the current implementatin this is dealt with by calling
> >>     OVS_NOT_REACHED() and terminating OvS.
> >>
> >>     To avoid this, and allow the (already) allocated space to be
> better
> >>     used, dp_packet_resize__() now tries to use the available
> room, both the
> >>     tailroom and the headroom, to make enough space for the new
> data. Since
> >>     this happens for packets of source DPBUF_DPDK, the
> single-segment mbuf
> >>     case mentioned above is also covered by this new aproach in
> resize__().
> >>
> >>     Signed-off-by: Tiago Lam  
> >>     >>
> >>     Acked-by: Eelco Chaudron  
> >>     >>
> >>     ---
> >>      lib/dp-packet.c | 48
> ++--
> >>      1 file changed, 46 insertions(+), 2 deletions(-)
> >>
> >>     diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> >>     index d6e19eb..87af459 100644
> >>     --- a/lib/dp-packet.c
> >>     +++ b/lib/dp-packet.c
> >>     @@ -237,9 +237,51 @@ dp_packet_resize__(struct dp_packet *b,
> size_t
> >>     new_headroom, size_t new_tailroom
> >>          new_allocated = new_headroom + dp_packet_size(b) +
> new_tailroom;
> >>
> >>          switch (b->source) {
> >>     +    /* When resizing mbufs, both a single mbuf and multi-segment
> >>     mbufs (where
> >>     +     * data is not contigously held in memory), both the
> headroom
> >>     and the
> >>     +     * tailroom available will be used to make more space
> for where
> >>     data needs
> >>     +     * to be inserted. I.e if there's not enough headroom,
> data may
> >>     be shifted
> >>     +     * right if there's enough tailroom.
> >>     +     * However, this is not bulletproof and in some cases
> the space
> >>     available
> >>     +     * won't be enough - in those cases, an error should be
> >>     returned and the
> >>     +     * packet dropped. */
> >>          case DPBUF_DPDK:
> >>     -        OVS_NOT_REACHED();
> >>
> >>
> >> Previously, it was a coding error to call this function for a
> DPDK mbuf
> >> case, which is pretty
> >> clear. But with this patch, presumably that is not longer the
> case and
> >> the calling the API is
> >> now ok for DPDK mbufs.
> >>
> >
> > As it stands, it will still be an error to call
> dp_packet_resize__() for
> > any DPDK packet, or by extension any of the other functions that call
> > it, such as dp_packet_prealloc_tailroom() and
> > dp_packet_prealloc_headroom(). This patch only tries to alleviate that
> > by accommodating space from the headroom or tailroom, if possible, and
> > create just enough space for the new data. My preferred approach would
> > be to return an error if not possible, but since the API doesn't deal
> > with errors as is, the previous behavior of manually asserting was
> left
> > as is. As reported in [1] (I comment more on that below), the behavior
> > of manually asserting can lead to undesired behavior in some use
> cases.
> >
> >>  
> >>
> >>     +    {
> >>     +        size_t miss_len;
> >>     +
> >>     +        if (new_headroom == dp_packet_headroom(b)) {
> >>     +            /* This is a tailroom adjustment. Since there's no
> >>     tailroom space
> >>     +             * left, try and shift data towards the head to
> free up
> >>     tail space,
> >>     +             * if there's enough headroom */
> >>     +
> >>    

Re: [ovs-dev] [PATCH v5 08/14] dp-packet: Handle multi-seg mbufs in resize__().

[Lam, Tiago]

On 18/07/2018 23:53, Darrell Ball wrote:
> sorry, several distractions delayed response.
> 
> On Mon, Jul 16, 2018 at 1:37 AM, Lam, Tiago  > wrote:
> 
> On 13/07/2018 18:54, Darrell Ball wrote:
> > Thanks for the patch.
> > 
> > A few queries inline.
> > 
> 
> Hi Darrell,
> 
> Thanks for your inputs. I've replied in-line as well.
> 
> > On Wed, Jul 11, 2018 at 11:23 AM, Tiago Lam  
> > >> wrote:
> > 
> >     When enabled with DPDK OvS relies on mbufs allocated by mempools to
> >     receive and output data on DPDK ports. Until now, each OvS 
> dp_packet has
> >     had only one mbuf associated, which is allocated with the maximum
> >     possible size, taking the MTU into account. This approach, however,
> >     doesn't allow us to increase the allocated size in an mbuf, if 
> needed,
> >     since an mbuf is allocated and initialised upon mempool creation. 
> Thus,
> >     in the current implementatin this is dealt with by calling
> >     OVS_NOT_REACHED() and terminating OvS.
> > 
> >     To avoid this, and allow the (already) allocated space to be better
> >     used, dp_packet_resize__() now tries to use the available room, 
> both the
> >     tailroom and the headroom, to make enough space for the new data. 
> Since
> >     this happens for packets of source DPBUF_DPDK, the single-segment 
> mbuf
> >     case mentioned above is also covered by this new aproach in 
> resize__().
> > 
> >     Signed-off-by: Tiago Lam  
> >     >>
> >     Acked-by: Eelco Chaudron  
> >     >>
> >     ---
> >      lib/dp-packet.c | 48
> ++--
> >      1 file changed, 46 insertions(+), 2 deletions(-)
> >
> >     diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> >     index d6e19eb..87af459 100644
> >     --- a/lib/dp-packet.c
> >     +++ b/lib/dp-packet.c
> >     @@ -237,9 +237,51 @@ dp_packet_resize__(struct dp_packet *b,
> size_t
> >     new_headroom, size_t new_tailroom
> >          new_allocated = new_headroom + dp_packet_size(b) +
> new_tailroom;
> >
> >          switch (b->source) {
> >     +    /* When resizing mbufs, both a single mbuf and multi-segment
> >     mbufs (where
> >     +     * data is not contigously held in memory), both the headroom
> >     and the
> >     +     * tailroom available will be used to make more space for
> where
> >     data needs
> >     +     * to be inserted. I.e if there's not enough headroom,
> data may
> >     be shifted
> >     +     * right if there's enough tailroom.
> >     +     * However, this is not bulletproof and in some cases the
> space
> >     available
> >     +     * won't be enough - in those cases, an error should be
> >     returned and the
> >     +     * packet dropped. */
> >          case DPBUF_DPDK:
> >     -        OVS_NOT_REACHED();
> >
> >
> > Previously, it was a coding error to call this function for a DPDK
> mbuf
> > case, which is pretty
> > clear. But with this patch, presumably that is not longer the case and
> > the calling the API is
> > now ok for DPDK mbufs.
> >
> 
> As it stands, it will still be an error to call dp_packet_resize__() for
> any DPDK packet, or by extension any of the other functions that call
> it, such as dp_packet_prealloc_tailroom() and
> dp_packet_prealloc_headroom(). 
> 
> 
> 
> yep, the existing code fails in a very clear way; i.e. whenever it is
> called for a dpdk packet.
> So the user would need to handle in some other way, which is not being
> done today, I know.
> 
>  
> 
> This patch only tries to alleviate that
> by accommodating space from the headroom or tailroom, if possible, and
> create just enough space for the new data. 
> 
> 
> The new code will fail is some yet undefined way, occasionally working
> and failing
> in the "other" cases.
> 
>  
> 
> My preferred approach would
> be to return an error if not possible, but since the API doesn't deal
> with errors as is, the previous behavior of manually asserting was left
> as is. As reported in [1] (I comment more on that below), the behavior 
> 
> of manually asserting can lead to undesired behavior in some use cases.
> 
> 
> 
> I am familiar with the issue.
> As part of the change,  dp_packet_put_uninit()
> and dp_packet_push_uninit() could be modified to return NULL
> and that could be percolated and checked for.
> 
> Those APIs could simply check (by calling a helper API) if they would
> fail a priori to 

Re: [ovs-dev] [PATCH] ofproto: Return error codes for Rule insertions

[Ben Pfaff]

It is super annoying to send a nagging message every day.  Do not do it.

On Fri, Jul 13, 2018 at 08:03:22AM +0530, Aravind Prasad wrote:
> Hi  Aaron/Ben/All,
> If possible, Kindly review the patch and let me know
> your views.
> 
> On Thu, Jul 12, 2018 at 11:34 PM, Aravind Prasad S 
> wrote:
> 
> > Currently, rule_insert() API does not have return value. There are some
> > possible
> > scenarios where rule insertions can fail at run-time even though the static
> > checks during rule_construct() had passed previously.
> > Some possible scenarios for failure of rule insertions:
> > **) Rule insertions can fail dynamically in Hybrid mode (both Openflow and
> > Normal switch functioning coexist) where the CAM space could get suddenly
> > filled up by Normal switch functioning and Openflow gets devoid of
> > available space.
> > **) Some deployments could have separate independent layers for HW rule
> > insertions and application layer to interact with OVS. HW layer
> > could face any dynamic issue during rule handling which application could
> > not have predicted/captured in rule-construction phase.
> > Rule-insert errors for bundles are handled too in this pull-request.
> >
> > Signed-off-by: Aravind Prasad S 
> > ---
> >  ofproto/ofproto-dpif.c |   4 +-
> >  ofproto/ofproto-provider.h |   6 +--
> >  ofproto/ofproto.c  | 105 ++
> > ---
> >  3 files changed, 85 insertions(+), 30 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index ad1e8af..d1678ed 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -4443,7 +4443,7 @@ rule_construct(struct rule *rule_)
> >  return 0;
> >  }
> >
> > -static void
> > +static enum ofperr
> >  rule_insert(struct rule *rule_, struct rule *old_rule_, bool
> > forward_counts)
> >  OVS_REQUIRES(ofproto_mutex)
> >  {
> > @@ -4473,6 +4473,8 @@ rule_insert(struct rule *rule_, struct rule
> > *old_rule_, bool forward_counts)
> >  ovs_mutex_unlock(>stats_mutex);
> >  ovs_mutex_unlock(_rule->stats_mutex);
> >  }
> > +
> > +return 0;
> >  }
> >
> >  static void
> > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> > index 2b77b89..3f3d110 100644
> > --- a/ofproto/ofproto-provider.h
> > +++ b/ofproto/ofproto-provider.h
> > @@ -1297,8 +1297,8 @@ struct ofproto_class {
> >  struct rule *(*rule_alloc)(void);
> >  enum ofperr (*rule_construct)(struct rule *rule)
> >  /* OVS_REQUIRES(ofproto_mutex) */;
> > -void (*rule_insert)(struct rule *rule, struct rule *old_rule,
> > -bool forward_counts)
> > +enum ofperr (*rule_insert)(struct rule *rule, struct rule *old_rule,
> > +   bool forward_counts)
> >  /* OVS_REQUIRES(ofproto_mutex) */;
> >  void (*rule_delete)(struct rule *rule) /* OVS_REQUIRES(ofproto_mutex)
> > */;
> >  void (*rule_destruct)(struct rule *rule);
> > @@ -1952,7 +1952,7 @@ enum ofperr ofproto_flow_mod_learn_start(struct
> > ofproto_flow_mod *ofm)
> >  OVS_REQUIRES(ofproto_mutex);
> >  void ofproto_flow_mod_learn_revert(struct ofproto_flow_mod *ofm)
> >  OVS_REQUIRES(ofproto_mutex);
> > -void ofproto_flow_mod_learn_finish(struct ofproto_flow_mod *ofm,
> > +enum ofperr ofproto_flow_mod_learn_finish(struct ofproto_flow_mod *ofm,
> >struct ofproto *orig_ofproto)
> >  OVS_REQUIRES(ofproto_mutex);
> >  void ofproto_add_flow(struct ofproto *, const struct match *, int
> > priority,
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index f946e27..cb09ee6 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -245,10 +245,12 @@ static void replace_rule_revert(struct ofproto *,
> > struct rule *old_rule,
> >  struct rule *new_rule)
> >  OVS_REQUIRES(ofproto_mutex);
> >
> > -static void replace_rule_finish(struct ofproto *, struct ofproto_flow_mod
> > *,
> > -const struct openflow_mod_requester *,
> > -struct rule *old_rule, struct rule
> > *new_rule,
> > -struct ovs_list *dead_cookies)
> > +static enum ofperr replace_rule_finish(struct ofproto *,
> > +   struct ofproto_flow_mod *,
> > +   const struct
> > openflow_mod_requester *,
> > +   struct rule *old_rule,
> > +   struct rule *new_rule,
> > +   struct ovs_list *dead_cookies)
> >  OVS_REQUIRES(ofproto_mutex);
> >  static void delete_flows__(struct rule_collection *,
> > enum ofp_flow_removed_reason,
> > @@ -270,7 +272,7 @@ static enum ofperr ofproto_flow_mod_start(struct
> > ofproto *,
> >  static void ofproto_flow_mod_revert(struct ofproto *,
> > 

Re: [ovs-dev] [PATCH] ofproto: Return error codes for Rule insertions

[Ben Pfaff]

On Thu, Jul 12, 2018 at 06:04:47PM +, Aravind Prasad S wrote:
> Currently, rule_insert() API does not have return value. There are some 
> possible
> scenarios where rule insertions can fail at run-time even though the static
> checks during rule_construct() had passed previously.
> Some possible scenarios for failure of rule insertions:
> **) Rule insertions can fail dynamically in Hybrid mode (both Openflow and
> Normal switch functioning coexist) where the CAM space could get suddenly
> filled up by Normal switch functioning and Openflow gets devoid of
> available space.
> **) Some deployments could have separate independent layers for HW rule
> insertions and application layer to interact with OVS. HW layer
> could face any dynamic issue during rule handling which application could
> not have predicted/captured in rule-construction phase.
> Rule-insert errors for bundles are handled too in this pull-request.
> 
> Signed-off-by: Aravind Prasad S 

Which switches does this help?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] OVS DPDK: dpdk_merge pull request for master

[Ian Stokes]

Hi Ben,

The following changes since commit 
3c921cc2b6b760bd0db73fd629ee9614edc8914c:


  build: Add gitattribute file to build-aux (2018-07-19 21:02:33 +0300)

are available in the git repository at:

  https://github.com/istokes/ovs dpdk_merge

for you to fetch changes up to 0e0e9e213f13be508d282ffefd7bbe8c680e4fc8:

  sparse: Add support for DPDK. (2018-07-20 15:44:45 +0100)


Ben Pfaff (4):
  netdev-dpdk: Fix incorrect byte order conversion in log message.
  netdev-dpdk: Fix sparse complaints.
  netdev-dpdk: Use ETH_ADDR_BYTES_ARGS instead of open-coding it.
  sparse: Add support for DPDK.

Ian Stokes (1):
  Docs: Improve OVS DPDK version mapping notice.

Mark Kavanagh (4):
  netdev-dpdk: fix mbuf sizing
  dp-packet: Init specific mbuf fields.
  netdev-dpdk: copy large packet to multi-seg. mbufs
  netdev-dpdk: support multi-segment jumbo frames

Michael Qiu (1):
  dp-packet: copy data from multi-seg. DPDK mbuf

Tiago Lam (9):
  dp-packet: Fix allocated size on DPDK init.
  netdev-dpdk: Serialise non-pmds mbufs' alloc/free.
  dp-packet: Fix data_len handling multi-seg mbufs.
  dp-packet: Handle multi-seg mbufs in helper funcs.
  dp-packet: Handle multi-seg mubfs in shift() func.
  dp-packet: Handle multi-seg mbufs in resize__().
  dpdk-tests: Add uni-tests for multi-seg mbufs.
  dpdk-tests: Accept other configs in OVS_DPDK_START
  dpdk-tests: End-to-end tests for multi-seg mbufs.

Yipeng Wang (1):
  dpif-netdev: Add SMC cache after EMC cache

 Documentation/howto/dpdk.rst   |6 +-
 Documentation/intro/install/dpdk.rst   |6 +-
 Documentation/topics/dpdk/bridge.rst   |   15 ++
 Documentation/topics/dpdk/jumbo-frames.rst |   52 +++
 Documentation/topics/dpdk/memory.rst   |   36 +
 Makefile.am|2 +-
 NEWS   |3 +
 build-aux/initial-tab-whitelist|1 +
 include/sparse/automake.mk |9 ++
 include/sparse/rte_byteorder.h |  281 
+++

 include/sparse/rte_esp.h   |   65 +
 include/sparse/rte_flow.h  | 1483 
+

 include/sparse/rte_icmp.h  |  106 ++
 include/sparse/rte_ip.h|  490 
+

 include/sparse/rte_sctp.h  |  103 +
 include/sparse/rte_tcp.h   |  108 ++
 include/sparse/rte_udp.h   |  103 +
 include/sparse/xmmintrin.h |   24 +++
 lib/cmap.c |   74 ++
 lib/cmap.h |   11 ++
 lib/dp-packet.c|  221 
++--
 lib/dp-packet.h|  214 
---

 lib/dpdk.c |8 +
 lib/dpif-netdev-perf.h |1 +
 lib/dpif-netdev.c  |  329 
-
 lib/netdev-dpdk.c  |  270 
+++---

 lib/netdev-dpdk.h  |2 +
 tests/automake.mk  |   10 +-
 tests/dpdk-packet-mbufs.at |7 +
 tests/pmd.at   |7 +-
 tests/system-dpdk-macros.at|6 +-
 tests/system-dpdk-testsuite.at |1 +
 tests/system-dpdk.at   |   65 +
 tests/test-dpdk-mbufs.c|  518 
+

 vswitchd/vswitch.xml   |   35 +
 35 files changed, 4532 insertions(+), 140 deletions(-)
 create mode 100644 include/sparse/rte_byteorder.h
 create mode 100644 include/sparse/rte_esp.h
 create mode 100644 include/sparse/rte_flow.h
 create mode 100644 include/sparse/rte_icmp.h
 create mode 100644 include/sparse/rte_ip.h
 create mode 100644 include/sparse/rte_sctp.h
 create mode 100644 include/sparse/rte_tcp.h
 create mode 100644 include/sparse/rte_udp.h
 create mode 100644 include/sparse/xmmintrin.h
 create mode 100644 tests/dpdk-packet-mbufs.at
 create mode 100644 tests/test-dpdk-mbufs.c

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

Re: [ovs-dev] OVS DPDK: dpdk_merge pull request for master

[Ian Stokes]

On 7/19/2018 7:06 PM, Ian Stokes wrote:

On 7/13/2018 5:56 PM, Ian Stokes wrote:

Hi Ben,

The following changes since commit 
89dd5819cf181a741271d297bc99fea4760f7ba5:


   rhel: support kmod-openvswitch build against multiple kernels, 
rhel6 (2018-07-12 17:42:07 -0700)


are available in the git repository at:

   https://github.com/istokes/ovs dpdk_merge

for you to fetch changes up to 70f4d53a17d953b0fadf18361b54ce95550ebfb7:

   netdev-dpdk: Fix failure to configure flow control at netdev-init. 
(2018-07-13 17:08:56 +0100)


Hi Ben,

would it be easier for me to rebase this to the head of master and 
submit a new pull request with the sparse fixes you submitted as part of 
it also? Whatever you think is easiest.


Thanks
Ian


Hi Ben,

you can disregard this pull request, there is a patch that I want to 
remove as it introduces a bug.


I'll send a new pull request updated to the head of master for today 
instead, this then can be merged to master and be part of the 2.10 
release also.


Thanks
Ian



Ian Stokes (1):
   Docs: Improve OVS DPDK version mapping notice.

Mark Kavanagh (4):
   netdev-dpdk: fix mbuf sizing
   dp-packet: Init specific mbuf fields.
   netdev-dpdk: copy large packet to multi-seg. mbufs
   netdev-dpdk: support multi-segment jumbo frames

Michael Qiu (1):
   dp-packet: copy data from multi-seg. DPDK mbuf

Sugesh Chandran (1):
   netdev-dpdk: Fix failure to configure flow control at netdev-init.

Tiago Lam (9):
   dp-packet: Fix allocated size on DPDK init.
   netdev-dpdk: Serialise non-pmds mbufs' alloc/free.
   dp-packet: Fix data_len handling multi-seg mbufs.
   dp-packet: Handle multi-seg mbufs in helper funcs.
   dp-packet: Handle multi-seg mubfs in shift() func.
   dp-packet: Handle multi-seg mbufs in resize__().
   dpdk-tests: Add uni-tests for multi-seg mbufs.
   dpdk-tests: Accept other configs in OVS_DPDK_START
   dpdk-tests: End-to-end tests for multi-seg mbufs.

Yipeng Wang (1):
   dpif-netdev: Add SMC cache after EMC cache

  Documentation/howto/dpdk.rst   |   6 ++-
  Documentation/intro/install/dpdk.rst   |   6 ++-
  Documentation/topics/dpdk/bridge.rst   |  15 ++
  Documentation/topics/dpdk/jumbo-frames.rst |  52 +++
  Documentation/topics/dpdk/memory.rst   |  36 +
  NEWS   |   3 ++
  lib/cmap.c |  74 
+++

  lib/cmap.h |  11 
  lib/dp-packet.c    | 221 
+-- 

  lib/dp-packet.h    | 214 
+ 


  lib/dpdk.c |   8 +++
  lib/dpif-netdev-perf.h |   1 +
  lib/dpif-netdev.c  | 329 
-- 

  lib/netdev-dpdk.c  | 259 
- 


  lib/netdev-dpdk.h  |   2 +
  tests/automake.mk  |  10 +++-
  tests/dpdk-packet-mbufs.at |   7 +++
  tests/pmd.at   |   7 ++-
  tests/system-dpdk-macros.at    |   6 ++-
  tests/system-dpdk-testsuite.at |   1 +
  tests/system-dpdk.at   |  65 

  tests/test-dpdk-mbufs.c    | 518 
++ 


  vswitchd/vswitch.xml   |  35 +
  23 files changed, 1756 insertions(+), 130 deletions(-)
  create mode 100644 tests/dpdk-packet-mbufs.at
  create mode 100644 tests/test-dpdk-mbufs.c

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


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

Re: [ovs-dev] [PATCH v4] ovn: Allow for automatic dynamic updates of IPAM

[Mark Michelson]

On 07/20/2018 08:06 AM, Jakub Sitnicki wrote:

On Fri, Jul 13, 2018 at 06:45 PM GMT, Mark Michelson wrote:

OVN offers a method of IP address management that allows for an IPv4 subnet or
IPv6 prefix to be specified on a logical switch. Then by specifying a
switch port's address as "dynamic" or " dynamic", OVN will
automatically assign addresses to the switch port.

While this works great for initial assignment of addresses, addresses do
not automatically adjust when changes are made to the switch's
configuration. For instance:
* If the subnet, ipv6_prefix, or exclude_ips for a logical switch
changes, the affected switch ports are not updated.
* If a switch port with a static IP address is added to the switch, and
that address conflicts with a dynamically assigned IP address, the
dynamic address is not updated.
* If a MAC address switched from being statically assigned to
dynamically assigned, the MAC address would not be updated.
* If a statically assigned MAC address changed, then the IPv6 address
would not be updated.

This patch solves all of the above issues by changing the algorithm for
IPAM assignment. There are essentially three steps.
1) While joining logical ports, all statically-assigned addresses (i.e.
any ports without "dynamic" addresses) have their addresses registered
to IPAM. This gives them top priority.
2) All logical ports with dynamic addresses are inspected. Any changes
that must be made to the addresses are collected to be made later. Any
addresses that do not require change are registered to IPAM. This allows
for previously assigned dynamic addresses to be kept.
3) All gathered changes are enacted.

The change contains new tests that ensure that dynamic addresses are
updated when appropriate.

This patch also alters some existing IPAM tests. Those tests assumed
that dynamic addresses would not be updated automatically, so those
tests either had to be altered or removed.

Signed-off-by: Mark Michelson 


I've come up with a couple suggestions (see in-line) but otherwise LGTM:

Acked-by: Jakub Sitnicki 


Thanks for the review Jakub! These all look like good suggestions, so 
I'll put up a new version with those suggestions.





---
v3->v4:
  Print warning when multiple dynamic addresses are configured on a
  switch port. Ensure that dynamic addresses beyond the first on a switch
  port are ignored. Found by Ben Pfaff.

v2->v3:
  Fixed a checkpatch problem (line too long)

v1->v2:
  Rebased
---
  ovn/northd/ovn-northd.c | 385 ++--
  tests/ovn.at|  97 +---
  2 files changed, 350 insertions(+), 132 deletions(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 04a072ba8..b75febb44 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -985,9 +985,6 @@ ipam_add_port_addresses(struct ovn_datapath *od, struct 
ovn_port *op)
  for (size_t i = 0; i < op->nbsp->n_addresses; i++) {
  ipam_insert_lsp_addresses(od, op, op->nbsp->addresses[i]);
  }
-if (op->nbsp->dynamic_addresses) {
-ipam_insert_lsp_addresses(od, op, op->nbsp->dynamic_addresses);
-}
  } else if (op->nbrp) {
  struct lport_addresses lrp_networks;
  if (!extract_lrp_networks(op->nbrp, _networks)) {
@@ -1060,64 +1057,255 @@ ipam_get_unused_ip(struct ovn_datapath *od)
  return od->ipam_info.start_ipv4 + new_ip_index;
  }

+enum dynamic_update_type {
+NONE,/* No change to the address */
+REMOVE,  /* Address is no longer dynamic */
+STATIC,  /* Use static address (MAC only) */
+DYNAMIC, /* Assign a new dynamic address */
+};
+
+struct dynamic_address_update {
+struct ovs_list node;   /* In build_ipam()'s list of updates. */
+
+struct ovn_port *op;
+
+struct lport_addresses current_addresses;
+struct eth_addr static_mac;
+enum dynamic_update_type mac;
+enum dynamic_update_type ipv4;
+enum dynamic_update_type ipv6;
+};
+
+static enum dynamic_update_type
+dynamic_mac_changed(const char *lsp_addresses,
+struct dynamic_address_update *update)
+{
+   struct eth_addr ea;
+
+   if (ovs_scan(lsp_addresses, ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(ea))) {
+   if (eth_addr_equals(ea, update->current_addresses.ea)) {
+   return NONE;
+   } else {
+   /* MAC is still static, but it has changed */
+   update->static_mac = ea;
+   return STATIC;
+   }
+   }
+
+   uint64_t mac64 = eth_addr_to_uint64(update->current_addresses.ea);
+   if ((mac64 ^ MAC_ADDR_PREFIX) >> 24) {
+   return DYNAMIC;
+   } else {
+   return NONE;
+   }
+}
+
+static enum dynamic_update_type
+dynamic_ip4_changed(struct dynamic_address_update *update)
+{
+bool dynamic_ip4 = update->op->od->ipam_info.allocated_ipv4s != NULL;
+
+if (!dynamic_ip4) {
+if (update->current_addresses.n_ipv4_addrs) {
+return REMOVE;
+} else {
+return NONE;
+

Re: [ovs-dev] [PATCH v4] ovn: Allow for automatic dynamic updates of IPAM

[Jakub Sitnicki]

On Fri, Jul 13, 2018 at 06:45 PM GMT, Mark Michelson wrote:
> OVN offers a method of IP address management that allows for an IPv4 subnet or
> IPv6 prefix to be specified on a logical switch. Then by specifying a
> switch port's address as "dynamic" or " dynamic", OVN will
> automatically assign addresses to the switch port.
>
> While this works great for initial assignment of addresses, addresses do
> not automatically adjust when changes are made to the switch's
> configuration. For instance:
> * If the subnet, ipv6_prefix, or exclude_ips for a logical switch
> changes, the affected switch ports are not updated.
> * If a switch port with a static IP address is added to the switch, and
> that address conflicts with a dynamically assigned IP address, the
> dynamic address is not updated.
> * If a MAC address switched from being statically assigned to
> dynamically assigned, the MAC address would not be updated.
> * If a statically assigned MAC address changed, then the IPv6 address
> would not be updated.
>
> This patch solves all of the above issues by changing the algorithm for
> IPAM assignment. There are essentially three steps.
> 1) While joining logical ports, all statically-assigned addresses (i.e.
> any ports without "dynamic" addresses) have their addresses registered
> to IPAM. This gives them top priority.
> 2) All logical ports with dynamic addresses are inspected. Any changes
> that must be made to the addresses are collected to be made later. Any
> addresses that do not require change are registered to IPAM. This allows
> for previously assigned dynamic addresses to be kept.
> 3) All gathered changes are enacted.
>
> The change contains new tests that ensure that dynamic addresses are
> updated when appropriate.
>
> This patch also alters some existing IPAM tests. Those tests assumed
> that dynamic addresses would not be updated automatically, so those
> tests either had to be altered or removed.
>
> Signed-off-by: Mark Michelson 

I've come up with a couple suggestions (see in-line) but otherwise LGTM:

Acked-by: Jakub Sitnicki 

> ---
> v3->v4:
>  Print warning when multiple dynamic addresses are configured on a
>  switch port. Ensure that dynamic addresses beyond the first on a switch
>  port are ignored. Found by Ben Pfaff.
>
> v2->v3:
>  Fixed a checkpatch problem (line too long)
>
> v1->v2:
>  Rebased
> ---
>  ovn/northd/ovn-northd.c | 385 
> ++--
>  tests/ovn.at|  97 +---
>  2 files changed, 350 insertions(+), 132 deletions(-)
>
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 04a072ba8..b75febb44 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -985,9 +985,6 @@ ipam_add_port_addresses(struct ovn_datapath *od, struct 
> ovn_port *op)
>  for (size_t i = 0; i < op->nbsp->n_addresses; i++) {
>  ipam_insert_lsp_addresses(od, op, op->nbsp->addresses[i]);
>  }
> -if (op->nbsp->dynamic_addresses) {
> -ipam_insert_lsp_addresses(od, op, op->nbsp->dynamic_addresses);
> -}
>  } else if (op->nbrp) {
>  struct lport_addresses lrp_networks;
>  if (!extract_lrp_networks(op->nbrp, _networks)) {
> @@ -1060,64 +1057,255 @@ ipam_get_unused_ip(struct ovn_datapath *od)
>  return od->ipam_info.start_ipv4 + new_ip_index;
>  }
>
> +enum dynamic_update_type {
> +NONE,/* No change to the address */
> +REMOVE,  /* Address is no longer dynamic */
> +STATIC,  /* Use static address (MAC only) */
> +DYNAMIC, /* Assign a new dynamic address */
> +};
> +
> +struct dynamic_address_update {
> +struct ovs_list node;   /* In build_ipam()'s list of updates. */
> +
> +struct ovn_port *op;
> +
> +struct lport_addresses current_addresses;
> +struct eth_addr static_mac;
> +enum dynamic_update_type mac;
> +enum dynamic_update_type ipv4;
> +enum dynamic_update_type ipv6;
> +};
> +
> +static enum dynamic_update_type
> +dynamic_mac_changed(const char *lsp_addresses,
> +struct dynamic_address_update *update)
> +{
> +   struct eth_addr ea;
> +
> +   if (ovs_scan(lsp_addresses, ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(ea))) {
> +   if (eth_addr_equals(ea, update->current_addresses.ea)) {
> +   return NONE;
> +   } else {
> +   /* MAC is still static, but it has changed */
> +   update->static_mac = ea;
> +   return STATIC;
> +   }
> +   }
> +
> +   uint64_t mac64 = eth_addr_to_uint64(update->current_addresses.ea);
> +   if ((mac64 ^ MAC_ADDR_PREFIX) >> 24) {
> +   return DYNAMIC;
> +   } else {
> +   return NONE;
> +   }
> +}
> +
> +static enum dynamic_update_type
> +dynamic_ip4_changed(struct dynamic_address_update *update)
> +{
> +bool dynamic_ip4 = update->op->od->ipam_info.allocated_ipv4s != NULL;
> +
> +if (!dynamic_ip4) {
> +if (update->current_addresses.n_ipv4_addrs) {
> +return REMOVE;
>