Re: [ovs-dev] [PATCH ovn 11/11] ovs-sim: New utility for multi-node OVS and OVN simulation.
Really like this patch~!
> +Interconnection Network Commands
> +
> +
> + When multiple sandboxed Open vSwitch instances exist, one will
> inevitably
> + want to connect them together. These commands allow for that.
> + Conceptually, an interconnection network is a switch that
> + ovs-sim makes it easy to plug into other switches in
> other
> + sandboxed Open vSwitch instances. Interconnection networks are
> + implemented as bridges in the main switch that
> + ovs-sim creates by default, so to use interconnection
> + networks please avoid working with main directly.
> +
> +
> +
> + net_add network
> +
> +Creates a new interconnection network named network.
> +
> +
> + net_attach network
> bridge
> +
> +Adds a new port to bridge in the default sandbox (as
> set
> +with as) that plugs it into the network
> +interconnection network. network must already have
> been
> +created by a previous invocation of net_add. The
> default
> +sandbox must not be main.
> +
>
First sentence of the description of 'net_attach' is confusing. do you mean
's/that/and/'?
> diff --git a/utilities/ovs-sim.in b/utilities/ovs-sim.in
> new file mode 100755
> index 000..96d664b
> --- /dev/null
> +++ b/utilities/ovs-sim.in
> @@ -0,0 +1,368 @@
> ...
...
> +set -e
> +
> +sim_builddir='@abs_builddir@'; export sim_builddir
> +sim_srcdir='@abs_top_srcdir@'; export sim_srcdir
> +interactive=false
> +scripts=
> +
> +for option; do
> +# This option-parsing mechanism borrowed from a Autoconf-generated
> +# configure script under the following license:
> +
> +# Copyright (C) 1992, 1993, 1994, 1995, 1996, 1998, 1999, 2000, 2001,
> +# 2002, 2003, 2004, 2005, 2006, 2009, 2013 Free Software Foundation,
> Inc.
> +# This configure script is free software; the Free Software Foundation
> +# gives unlimited permission to copy, distribute and modify it.
> +
> +# If the previous option needs an argument, assign it.
> +if test -n "$prev"; then
> +eval $prev=\$option
> +prev=
> +continue
> +fi
>
"prev" not used, i think the above code can be removed?
> +case $option in
> +*=*) optarg=`expr "X$option" : '[^=]*=\(.*\)'` ;;
> +*) optarg=yes ;;
> +esac
> +
>
"optarg" also not used.
> +case $dashdash$option in
> +--)
> +dashdash=yes ;;
>
Not sure what '$dashdash' does here. But do not see it useful
> +-h|--help)
> +cat < +$0, for starting sandboxed dummy Open vSwitch environments
> +usage: $0 [OPTION...] [SCRIPT...]
> +
> +Options:
> + -i, --interactivePrompt for interactive input (default if no
> SCRIPTs)
> + -h, --help Print this usage message.
> +EOF
> +exit 0
> +;;
> +
> +-i|--i*)
> +interactive=:
> +;;
> +
> +-*)
> +echo "unrecognized option $option (use --help for help)" >&2
> +exit 1
> +;;
> +*)
> + case $option in
> + /*) ;;
> + *) option=`pwd`/$option ;;
> + esac
> +scripts="$scripts $option"
> +;;
> +esac
> +shift
> +done
> +
> +if test -z "$scripts"; then
> +interactive=:
> +fi
> +
> +# Check that we've got proper builddir and srcdir.
> +if test ! -e "$sim_builddir"/vswitchd/ovs-vswitchd; then
> +echo "$sim_builddir/vswitchd/ovs-vswitchd does not exist (need to run
> \"make\"?)" >&2
> +exit 1
> +fi
> +if test ! -e "$sim_srcdir"/WHY-OVS.md; then
> +echo "$sim_srcdir/WHY-OVS.md does not exist" >&2
> +exit 1
> +fi
> +
>
Why WHY-OVS? ;D
> +# Put built tools early in $PATH.
> +if test ! -e $sim_builddir/vswitchd/ovs-vswitchd; then
> +echo >&2 'build not found, please change set $sim_builddir or change
> directory'
> +exit 1
> +fi
>
Duplicated check?
>
> +PATH=$sim_builddir/ovsdb:$sim_builddir/vswitchd:$sim_builddir/utilities:$PATH
>
> +PATH=$sim_builddir/ovn:$sim_srcdir/ovn:$sim_builddir/ovn/controller:$sim_builddir/ovn/northd:$PATH
> +export PATH
> +
> +rm -rf sandbox
> +mkdir sandbox
> +cd sandbox
> +sim_base=`pwd`; export sim_base
> +trap "cd '$sim_base' && kill \`cat */*.pid\`" 0 1 2 3 13 14 15
> +
> +sim_setvars() {
> +sandbox=$1
> +OVS_RUNDIR=$sim_base/$1; export OVS_RUNDIR
> +OVS_LOGDIR=$sim_base/$1; export OVS_LOGDIR
> +OVS_DBDIR=$sim_base/$1; export OVS_DBDIR
> +OVS_SYSCONFDIR=$sim_base/$1; export OVS_SYSCONFDIR
> +PS1="|$1: $sim_PS1"
> +}
>
Do you want to add a space between for PS1? (i.e., PS1="| S1: $sim_PS1")
> +export -f sim_setvars
> +
> +as() {
> +case $# in
> + 0)
> +echo >&2 "$FUNCNAME: missing arguments (use --help for help)"
> +return 1
> + ;;
> + 1)
> + if test "$1" != --help; then
> + sim_setva
Re: [ovs-dev] [PATCH] dpif-netdev: Prefetch next packet before miniflow_extract().
>
> Acked-by: Ethan Jackson
>
> One question I had for a future patch. Have you considered prefetching
> more packets at once? I.E. 4 at a time or something?
> That's how these things are typically written, though I don't know if it would
> actually affect things.
yeah, I was going to suggest the same thing. In most DPDK apps, we would
prefetch about 3.
>
> Merged.
> Ethan
>
> On Mon, Jun 15, 2015 at 11:06 AM, Daniele Di Proietto
> wrote:
> > It appears that miniflow_extract() in emc_processing() spends a lot of
> > cycles waiting for the packet's data to be read.
> >
> > Prefetching the next packet's data while parsing removes this delay.
> > For a single flow pipeline the throughput improves by ~10%. With a
> > more realistic pipeline the change has a much smaller effect (~0.5%
> > improvement)
> >
> > Signed-off-by: Daniele Di Proietto
> > ---
> > lib/dpif-netdev.c | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
> > 5b82c8b..f13169c 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -3150,6 +3150,11 @@ emc_processing(struct dp_netdev_pmd_thread
> *pmd, struct dp_packet **packets,
> > continue;
> > }
> >
> > +if (i != cnt - 1) {
> > +/* Prefetch next packet data */
> > +OVS_PREFETCH(dp_packet_data(packets[i+1]));
> > +}
> > +
> > miniflow_extract(packets[i], &key.mf);
> > key.len = 0; /* Not computed yet. */
> > key.hash = dpif_netdev_packet_get_rss_hash(packets[i],
> > &key.mf);
> > --
> > 2.1.4
> >
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] datapath-windows: BSOD when disabling the extension
Hi Eitan, Please see below the stacktrace of the BSOD. The FilterDetach routine was called while the requests were being processed and the gOvsSwitchContext global pointer was set to NULL. In this case the gOvsSwitchContext was not released, but only the gOvsSwitchContextRefCount reference count was decreased. -Sorin *** * * *Bugcheck Analysis* * * *** SYSTEM_SERVICE_EXCEPTION (3b) An exception happened while executing a system service routine. Arguments: Arg1: c005, Exception code that caused the bugcheck Arg2: f800023e685b, Address of the instruction which caused the bugcheck Arg3: d000236adad0, Address of the context record for the exception that caused the bugcheck Arg4: , zero. Debugging Details: -- "KERNEL32.DLL" was not found in the image list. Debugger will attempt to load "KERNEL32.DLL" at given base `. Please provide the full image name, including the extension (i.e. kernel32.dll) for more reliable results.Base address and size overrides can be given as .reload =,. Unable to add module at ` EXCEPTION_CODE: (NTSTATUS) 0xc005 - The instruction at 0x%08lx referenced memory at 0x%08lx. The memory could not be %s. FAULTING_IP: OVSExt!OvsNewVportCmdHandler+27b [c:\1.data\cloudbase\work\git\ovs\datapath-windows\ovsext\vport.c @ 2136] f800`023e685b 488b4870mov rcx,qword ptr [rax+70h] CONTEXT: d000236adad0 -- (.cxr 0xd000236adad0;r) rax= rbx=e33517a0 rcx=e39688e4 rdx=d000236ae584 rsi=e2d93c90 rdi=e33517a0 rip=f800023e685b rsp=d000236ae500 rbp=d000236aeb80 r8= r9=f800023f0a50 r10=d00020b02f80 r11=d00020afec30 r12= r13=0001 r14=e33518b8 r15=e2e53920 iopl=0 nv up ei pl zr na po nc cs=0010 ss=0018 ds=002b es=002b fs=0053 gs=002b efl=00010246 OVSExt!OvsNewVportCmdHandler+0x27b: f800`023e685b 488b4870mov rcx,qword ptr [rax+70h] ds:002b:`0070= Last set context: rax= rbx=e33517a0 rcx=e39688e4 rdx=d000236ae584 rsi=e2d93c90 rdi=e33517a0 rip=f800023e685b rsp=d000236ae500 rbp=d000236aeb80 r8= r9=f800023f0a50 r10=d00020b02f80 r11=d00020afec30 r12= r13=0001 r14=e33518b8 r15=e2e53920 iopl=0 nv up ei pl zr na po nc cs=0010 ss=0018 ds=002b es=002b fs=0053 gs=002b efl=00010246 OVSExt!OvsNewVportCmdHandler+0x27b: f800`023e685b 488b4870mov rcx,qword ptr [rax+70h] ds:002b:`0070= Resetting default scope DEFAULT_BUCKET_ID: WIN8_DRIVER_FAULT BUGCHECK_STR: 0x3B PROCESS_NAME: ovs-vswitchd.e CURRENT_IRQL: 0 ANALYSIS_VERSION: 6.3.9600.17237 (debuggers(dbg).140716-0327) amd64fre LAST_CONTROL_TRANSFER: from f800023d67f6 to f800023e685b STACK_TEXT: d000`236ae500 f800`023d67f6 : d000`236ae7c0 d000`236ae728 e000` `0010 : OVSExt!OvsNewVportCmdHandler+0x27b [c:\1.data\cloudbase\work\git\ovs\datapath-windows\ovsext\vport.c @ 2136] d000`236ae630 f800`023f6aff : d000`236ae7c0 f800`023f2220 d000`236ae728 e000`039688c0 : OVSExt!InvokeNetlinkCmdHandler+0x106 [c:\1.data\cloudbase\work\git\ovs\datapath-windows\ovsext\datapath.c @ 1003] d000`236ae6b0 f800`0073bc18 : e000`02d93c90 e000`033517a0 e000`02e53920 e000`033517a0 : OVSExt!OvsDeviceControl+0x98f [c:\1.data\cloudbase\work\git\ovs\datapath-windows\ovsext\datapath.c @ 912] d000`236ae840 f803`8ce4f395 : e000`033517a0 `0001 e000`02e53920 `000e : NDIS!ndisDummyIrpHandler+0x88 d000`236ae870 f803`8ce4fd2a : e32b`7f20ffbd 000c`001f0003 `0001 ` : nt!IopXxxControlFile+0x845 d000`236aea20 f803`8cbe08b3 : ` ` `0001 f803` : nt!NtDeviceIoControlFile+0x56 d000`236aea90 `77a22772 : `77a22371 0023`77a6b63c `0023 `00ff : nt!KiSystemServiceCopyEnd+0x13 `00f1e8b8 `77a22371 : 0023`77a6b63c `0023 `00ff `0101ffdc : wow64cpu!CpupSyscallStub+0x2 `00f1e8c0 `7797323a : ` `77a21503 ` `77973420 : wow64cpu!DeviceIoctlFileFault+0x31 `00f1e970 `7797317e : ` `000
Re: [ovs-dev] [PATCH] ovs-vtep: Support userspace datapaths.
On 15/06/2015 19:28, "Gurucharan Shetty" wrote:
>On Mon, Jun 15, 2015 at 11:09 AM, Daniele Di Proietto
> wrote:
>> With this commit, the VTEP emulator detects the datapath_type of the
>> bridge used as a "physical" switch, and creates subsequent bridges
>> with the same type. This allows ovs-vtep to work with the userspace
>> datapath.
>>
>> Signed-off-by: Daniele Di Proietto
>> ---
>> vtep/ovs-vtep | 12 ++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/vtep/ovs-vtep b/vtep/ovs-vtep
>> index 60dbb95..49c323f 100755
>> --- a/vtep/ovs-vtep
>> +++ b/vtep/ovs-vtep
>> @@ -40,6 +40,7 @@ vlog = ovs.vlog.Vlog("ovs-vtep")
>> exiting = False
>>
>> ps_name = ""
>> +ps_type = ""
>> Tunnel_Ip = ""
>> Lswitches = {}
>> Bindings = {}
>> @@ -103,7 +104,8 @@ class Logical_Switch(object):
>> self.tunnel_key = 0
>> vlog.warn("invalid tunnel key for %s, using 0" % self.name)
>>
>> -ovs_vsctl("--may-exist add-br %s" % self.short_name)
>> +ovs_vsctl("--may-exist add-br %s -- set Bridge %s
>>datapath_type=%s"
>> + % (self.short_name, self.short_name, ps_type))
>> ovs_vsctl("br-set-external-id %s vtep_logical_switch true"
>>% self.short_name)
>> ovs_vsctl("br-set-external-id %s logical_switch_name %s"
>> @@ -595,6 +597,11 @@ def setup():
>> if (ps_name not in br_list):
>> ovs.util.ovs_fatal(0, "couldn't find OVS bridge %s" % ps_name,
>>vlog)
>>
>> +global ps_type
>> +ps_type = ovs_vsctl("get Bridge %s datapath_type" % ps_name)
>> +if ps_type == "":
>Not a python expert, but I think 'if not ps_type:' is the preferred way.
More expert than I am, since I was not aware of this!
I actually had to change that particular test, because it turns out that
ovs-vsctl is returning "\"\"" if datapath_type is not set, but I've used
your suggestion elsewhere in the code
>
>> +ovs.util.ovs_fatal(0, "couldn't find %s datapath_type" %
>>ps_name, vlog)
>Is datapath_type always populated automatically? When I create a
>bridge on tip of master, that column does not get populated.
You're right, thanks for noticing this. I'll send out a v2 that doesn't
set the
datapath_type if it's not set in the physical bridge.
Thanks for your input!
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovs-vtep: Support userspace datapaths.
On 15/06/2015 20:40, "Alex Wang" wrote: >I actually have a similar patch in my ovn for vtep series. but instead, I >added >a new command-line option for that... But goal is the same, => use >different >datapath type (dummy in my case). > >So, I'm fine with this change, and will drop mine, looks good, I had this for a long time on my branches and I thought you might find it useful. If you prefer another approach I will drop it. Thanks Alex and Ben for your input > >On Mon, Jun 15, 2015 at 11:36 AM, Ben Pfaff wrote: > >> On Mon, Jun 15, 2015 at 11:28:12AM -0700, Gurucharan Shetty wrote: >> > On Mon, Jun 15, 2015 at 11:09 AM, Daniele Di Proietto >> > wrote: >> > > With this commit, the VTEP emulator detects the datapath_type of the >> > > bridge used as a "physical" switch, and creates subsequent bridges >> > > with the same type. This allows ovs-vtep to work with the userspace >> > > datapath. >> > > >> > > Signed-off-by: Daniele Di Proietto >> >> ... >> >> > > +ovs.util.ovs_fatal(0, "couldn't find %s datapath_type" % >> ps_name, vlog) >> > Is datapath_type always populated automatically? When I create a >> > bridge on tip of master, that column does not get populated. >> >> Good catch, I think: it's not populated automatically, so treating "" as >> an error is probably a bad idea. >> ___ >> dev mailing list >> dev@openvswitch.org >> >>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailm >>an_listinfo_dev&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r= >>SmB5nZacmXNq0gKCC1s_Cw5yUNjxgD4v5kJqZ2uWLlE&m=DRpxd0AAOCFSEkAcAaKydYlirka >>4I7cNFBpy-xfX59M&s=caNG5Lx2tx3DfOkJNrQBhhvIslCb3qenFwzQDpm8Q-k&e= >> >___ >dev mailing list >dev@openvswitch.org >https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailma >n_listinfo_dev&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=Sm >B5nZacmXNq0gKCC1s_Cw5yUNjxgD4v5kJqZ2uWLlE&m=DRpxd0AAOCFSEkAcAaKydYlirka4I7 >cNFBpy-xfX59M&s=caNG5Lx2tx3DfOkJNrQBhhvIslCb3qenFwzQDpm8Q-k&e= ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2] ovs-vtep: Support userspace datapaths.
With this commit, the VTEP emulator detects the datapath_type of the
bridge used as a "physical" switch, and creates subsequent bridges
with the same type. This allows ovs-vtep to work with the userspace
datapath.
Signed-off-by: Daniele Di Proietto
---
v1 - v2:
Applied Guru's suggestions
* Fixed unspecified datapath_type
* Use more pythonic check for empty string
---
vtep/ovs-vtep | 19 +--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/vtep/ovs-vtep b/vtep/ovs-vtep
index 60dbb95..6e8d0db 100755
--- a/vtep/ovs-vtep
+++ b/vtep/ovs-vtep
@@ -40,6 +40,7 @@ vlog = ovs.vlog.Vlog("ovs-vtep")
exiting = False
ps_name = ""
+ps_type = ""
Tunnel_Ip = ""
Lswitches = {}
Bindings = {}
@@ -103,7 +104,12 @@ class Logical_Switch(object):
self.tunnel_key = 0
vlog.warn("invalid tunnel key for %s, using 0" % self.name)
-ovs_vsctl("--may-exist add-br %s" % self.short_name)
+if ps_type:
+ovs_vsctl("--may-exist add-br %s -- set Bridge %s datapath_type=%s"
+ % (self.short_name, self.short_name, ps_type))
+else:
+ovs_vsctl("--may-exist add-br %s" % self.short_name)
+
ovs_vsctl("br-set-external-id %s vtep_logical_switch true"
% self.short_name)
ovs_vsctl("br-set-external-id %s logical_switch_name %s"
@@ -595,6 +601,11 @@ def setup():
if (ps_name not in br_list):
ovs.util.ovs_fatal(0, "couldn't find OVS bridge %s" % ps_name, vlog)
+global ps_type
+ps_type = ovs_vsctl("get Bridge %s datapath_type" % ps_name)
+if ps_type == "\"\"":
+ps_type = ""
+
call_prog("vtep-ctl", ["set", "physical_switch", ps_name,
'description="OVS VTEP Emulator"'])
@@ -636,7 +647,11 @@ def setup():
ovs_vsctl("del-br %s" % br)
-ovs_vsctl("add-br %s" % bfd_bridge)
+if ps_type:
+ovs_vsctl("add-br %s -- set Bridge %s datapath_type=%s"
+ % (bfd_bridge, bfd_bridge, ps_type))
+else:
+ovs_vsctl("add-br %s" % bfd_bridge)
def main():
--
2.1.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] datapath-windows: BSOD when disabling the extension
Acked-by: Eitan Eliahu Thanks Sorin. Eitan -Original Message- From: Sorin Vinturis [mailto:svintu...@cloudbasesolutions.com] Sent: Tuesday, June 16, 2015 2:36 AM To: Eitan Eliahu; dev@openvswitch.org Subject: RE: [ovs-dev] [PATCH v2] datapath-windows: BSOD when disabling the extension Hi Eitan, Please see below the stacktrace of the BSOD. The FilterDetach routine was called while the requests were being processed and the gOvsSwitchContext global pointer was set to NULL. In this case the gOvsSwitchContext was not released, but only the gOvsSwitchContextRefCount reference count was decreased. -Sorin *** * * *Bugcheck Analysis* * * *** SYSTEM_SERVICE_EXCEPTION (3b) An exception happened while executing a system service routine. Arguments: Arg1: c005, Exception code that caused the bugcheck Arg2: f800023e685b, Address of the instruction which caused the bugcheck Arg3: d000236adad0, Address of the context record for the exception that caused the bugcheck Arg4: , zero. Debugging Details: -- "KERNEL32.DLL" was not found in the image list. Debugger will attempt to load "KERNEL32.DLL" at given base `. Please provide the full image name, including the extension (i.e. kernel32.dll) for more reliable results.Base address and size overrides can be given as .reload =,. Unable to add module at ` EXCEPTION_CODE: (NTSTATUS) 0xc005 - The instruction at 0x%08lx referenced memory at 0x%08lx. The memory could not be %s. FAULTING_IP: OVSExt!OvsNewVportCmdHandler+27b [c:\1.data\cloudbase\work\git\ovs\datapath-windows\ovsext\vport.c @ 2136] f800`023e685b 488b4870mov rcx,qword ptr [rax+70h] CONTEXT: d000236adad0 -- (.cxr 0xd000236adad0;r) rax= rbx=e33517a0 rcx=e39688e4 rdx=d000236ae584 rsi=e2d93c90 rdi=e33517a0 rip=f800023e685b rsp=d000236ae500 rbp=d000236aeb80 r8= r9=f800023f0a50 r10=d00020b02f80 r11=d00020afec30 r12= r13=0001 r14=e33518b8 r15=e2e53920 iopl=0 nv up ei pl zr na po nc cs=0010 ss=0018 ds=002b es=002b fs=0053 gs=002b efl=00010246 OVSExt!OvsNewVportCmdHandler+0x27b: f800`023e685b 488b4870mov rcx,qword ptr [rax+70h] ds:002b:`0070= Last set context: rax= rbx=e33517a0 rcx=e39688e4 rdx=d000236ae584 rsi=e2d93c90 rdi=e33517a0 rip=f800023e685b rsp=d000236ae500 rbp=d000236aeb80 r8= r9=f800023f0a50 r10=d00020b02f80 r11=d00020afec30 r12= r13=0001 r14=e33518b8 r15=e2e53920 iopl=0 nv up ei pl zr na po nc cs=0010 ss=0018 ds=002b es=002b fs=0053 gs=002b efl=00010246 OVSExt!OvsNewVportCmdHandler+0x27b: f800`023e685b 488b4870mov rcx,qword ptr [rax+70h] ds:002b:`0070= Resetting default scope DEFAULT_BUCKET_ID: WIN8_DRIVER_FAULT BUGCHECK_STR: 0x3B PROCESS_NAME: ovs-vswitchd.e CURRENT_IRQL: 0 ANALYSIS_VERSION: 6.3.9600.17237 (debuggers(dbg).140716-0327) amd64fre LAST_CONTROL_TRANSFER: from f800023d67f6 to f800023e685b STACK_TEXT: d000`236ae500 f800`023d67f6 : d000`236ae7c0 d000`236ae728 e000` `0010 : OVSExt!OvsNewVportCmdHandler+0x27b [c:\1.data\cloudbase\work\git\ovs\datapath-windows\ovsext\vport.c @ 2136] d000`236ae630 f800`023f6aff : d000`236ae7c0 f800`023f2220 d000`236ae728 e000`039688c0 : OVSExt!InvokeNetlinkCmdHandler+0x106 [c:\1.data\cloudbase\work\git\ovs\datapath-windows\ovsext\datapath.c @ 1003] d000`236ae6b0 f800`0073bc18 : e000`02d93c90 e000`033517a0 e000`02e53920 e000`033517a0 : OVSExt!OvsDeviceControl+0x98f [c:\1.data\cloudbase\work\git\ovs\datapath-windows\ovsext\datapath.c @ 912] d000`236ae840 f803`8ce4f395 : e000`033517a0 `0001 e000`02e53920 `000e : NDIS!ndisDummyIrpHandler+0x88 d000`236ae870 f803`8ce4fd2a : e32b`7f20ffbd 000c`001f0003 `0001 ` : nt!IopXxxControlFile+0x845 d000`236aea20 f803`8cbe08b3 : ` ` `0001 f803` : nt!NtDeviceIoControlFile+0x56 d000`236aea90 `77a22772 : `77a22371 0023`77a6b63c `0023 `00ff : nt!KiSystemServiceCopyEnd+0x13 `00f1e8b8 `77a22371 : 0023`77a6b63c 00
Re: [ovs-dev] [PATCH v2] ovs-vtep: Support userspace datapaths.
On Tue, Jun 16, 2015 at 6:20 AM, Daniele Di Proietto
wrote:
> With this commit, the VTEP emulator detects the datapath_type of the
> bridge used as a "physical" switch, and creates subsequent bridges
> with the same type. This allows ovs-vtep to work with the userspace
> datapath.
>
> Signed-off-by: Daniele Di Proietto
> ---
> v1 - v2:
> Applied Guru's suggestions
> * Fixed unspecified datapath_type
> * Use more pythonic check for empty string
> ---
> vtep/ovs-vtep | 19 +--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/vtep/ovs-vtep b/vtep/ovs-vtep
> index 60dbb95..6e8d0db 100755
> --- a/vtep/ovs-vtep
> +++ b/vtep/ovs-vtep
> @@ -40,6 +40,7 @@ vlog = ovs.vlog.Vlog("ovs-vtep")
> exiting = False
>
> ps_name = ""
> +ps_type = ""
> Tunnel_Ip = ""
> Lswitches = {}
> Bindings = {}
> @@ -103,7 +104,12 @@ class Logical_Switch(object):
> self.tunnel_key = 0
> vlog.warn("invalid tunnel key for %s, using 0" % self.name)
>
> -ovs_vsctl("--may-exist add-br %s" % self.short_name)
> +if ps_type:
> +ovs_vsctl("--may-exist add-br %s -- set Bridge %s
> datapath_type=%s"
> + % (self.short_name, self.short_name, ps_type))
> +else:
> +ovs_vsctl("--may-exist add-br %s" % self.short_name)
> +
> ovs_vsctl("br-set-external-id %s vtep_logical_switch true"
>% self.short_name)
> ovs_vsctl("br-set-external-id %s logical_switch_name %s"
> @@ -595,6 +601,11 @@ def setup():
> if (ps_name not in br_list):
> ovs.util.ovs_fatal(0, "couldn't find OVS bridge %s" % ps_name, vlog)
>
> +global ps_type
> +ps_type = ovs_vsctl("get Bridge %s datapath_type" % ps_name)
> +if ps_type == "\"\"":
> +ps_type = ""
Else where in the code, this is handled by calling strip().
e.g: port_type = ovs_vsctl("get Interface %s type"
% port).strip('"')
It sort of looks cleaner to me and provides a little more consistency.
Otherwise, I am happy with the change.
> +
> call_prog("vtep-ctl", ["set", "physical_switch", ps_name,
> 'description="OVS VTEP Emulator"'])
>
> @@ -636,7 +647,11 @@ def setup():
>
> ovs_vsctl("del-br %s" % br)
>
> -ovs_vsctl("add-br %s" % bfd_bridge)
> +if ps_type:
> +ovs_vsctl("add-br %s -- set Bridge %s datapath_type=%s"
> + % (bfd_bridge, bfd_bridge, ps_type))
> +else:
> +ovs_vsctl("add-br %s" % bfd_bridge)
>
>
> def main():
> --
> 2.1.4
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH ovn v2] ofctrl: Don't use designated initializers.
MSVC 2013 does not like designated initializers when
structs are initialized inside structs.
Apparently it has been fixed in MSVC 2015.
Signed-off-by: Gurucharan Shetty
---
ovn/controller/ofctrl.c | 44 ++--
1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index bc4e8d4..2f1767a 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -401,12 +401,12 @@ ofctrl_update_flows(void)
if (!d) {
/* Installed flow is no longer desirable. Delete it from the
* switch and from installed_flows. */
-struct ofputil_flow_mod fm = {
-.match = i->match,
-.priority = i->priority,
-.table_id = i->table_id,
-.command = OFPFC_DELETE_STRICT,
-};
+struct ofputil_flow_mod fm;
+memset(&fm, 0, sizeof fm);
+fm.match = i->match;
+fm.priority = i->priority;
+fm.table_id = i->table_id;
+fm.command = OFPFC_DELETE_STRICT;
queue_flow_mod(&fm);
ovn_flow_log(i, "removing");
@@ -416,14 +416,14 @@ ofctrl_update_flows(void)
if (!ofpacts_equal(i->ofpacts, i->ofpacts_len,
d->ofpacts, d->ofpacts_len)) {
/* Update actions in installed flow. */
-struct ofputil_flow_mod fm = {
-.match = i->match,
-.priority = i->priority,
-.table_id = i->table_id,
-.ofpacts = d->ofpacts,
-.ofpacts_len = d->ofpacts_len,
-.command = OFPFC_MODIFY_STRICT,
-};
+struct ofputil_flow_mod fm;
+memset(&fm, 0, sizeof fm);
+fm.match = i->match;
+fm.priority = i->priority;
+fm.table_id = i->table_id;
+fm.ofpacts = d->ofpacts;
+fm.ofpacts_len = d->ofpacts_len;
+fm.command = OFPFC_MODIFY_STRICT;
queue_flow_mod(&fm);
ovn_flow_log(i, "updating");
@@ -446,14 +446,14 @@ ofctrl_update_flows(void)
struct ovn_flow *d;
HMAP_FOR_EACH_SAFE (d, next, hmap_node, &desired_flows) {
/* Send flow_mod to add flow. */
-struct ofputil_flow_mod fm = {
-.match = d->match,
-.priority = d->priority,
-.table_id = d->table_id,
-.ofpacts = d->ofpacts,
-.ofpacts_len = d->ofpacts_len,
-.command = OFPFC_ADD,
-};
+struct ofputil_flow_mod fm;
+memset(&fm, 0, sizeof fm);
+fm.match = d->match;
+fm.priority = d->priority;
+fm.table_id = d->table_id;
+fm.ofpacts = d->ofpacts;
+fm.ofpacts_len = d->ofpacts_len;
+fm.command = OFPFC_ADD;
queue_flow_mod(&fm);
ovn_flow_log(d, "adding");
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] IGMPv3 support
On Mon, Jun 08, 2015 at 01:05:41PM -0300, Thadeu Lima de Souza Cascardo wrote: > Support IGMPv3 messages with multiple records. Make sure all IGMPv3 > messages go through slow path, since they may carry multiple multicast > addresses, unlike IGMPv2. > > Tests done: > > * multiple addresses in IGMPv3 report are inserted in mdb; > * address is removed from IGMPv3 if record is INCLUDE_MODE; > * reports sent on a burst with same flow all go to userspace; > * IGMPv3 reports go to mrouters, i.e., ports that have issued a query. > > Signed-off-by: Thadeu Lima de Souza Cascardo Thanks! I have a few comments. I am surprised that igmpv3_header in packets.h is marked OVS_PACKED, because it looks like all of the fields are naturally aligned. Does this structure need to be packed? Similarly for igmpv3_record. However, in this struct I would also change 'maddr' from ovs_be32 to ovs_16aligned_be32, because there are situations where packet headers can be aligned on an odd 16-bit boundary and using ovs_16aligned_be32 makes it easier to handle that safely on RISC architectures. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH ovn 09/11] xml2nroff: Add support for variable substitutions.
On Mon, Jun 15, 2015 at 01:22:59PM -0700, Alex Wang wrote:
> On Sun, Jun 14, 2015 at 12:19 PM, Ben Pfaff wrote:
>
> > This allows XML-generated manpages in the source tree to include correct
> > directory names for the local configuration, instead of just the plain
> > nroff ones.
> >
> > Signed-off-by: Ben Pfaff
> > ---
> > build-aux/xml2nroff | 30 +-
> > ovn/automake.mk | 17 +++--
> > 2 files changed, 40 insertions(+), 7 deletions(-)
> >
> > diff --git a/build-aux/xml2nroff b/build-aux/xml2nroff
> > index 8dc9d4f..2dbbe50 100755
> > --- a/build-aux/xml2nroff
> > +++ b/build-aux/xml2nroff
> > @@ -28,17 +28,26 @@ def usage():
> > print """\
> > %(argv0)s: XML to nroff converter
> > Converts the XML format supplied as input into an nroff-formatted manpage.
> > -usage: %(argv0)s [OPTIONS] INPUT.XML
> > +usage: %(argv0)s [OPTIONS] INPUT.XML [VAR=VALUE]...
> > where INPUT.XML is a manpage in an OVS-specific XML format.
> >
> > +Each VAR, when enclosed by "@"s in the input, is replaced by its
> > +corresponding VALUE, with characters &<>"' in VALUE escaped.
> > +
> > The following options are also available:
> >--version=VERSION use VERSION to display on document footer
> >-h, --help display this help message\
> > """ % {'argv0': argv0}
> > sys.exit(0)
> >
> > -def manpage_to_nroff(xml_file, version=None):
> > -doc = xml.dom.minidom.parse(xml_file).documentElement
> > +def manpage_to_nroff(xml_file, subst, version=None):
> > +f = open(xml_file)
> > +input = []
> > +for line in f:
> > +for k, v in subst.iteritems():
> > +line = line.replace(k, v)
> > +input += [line]
> > +doc = xml.dom.minidom.parseString(''.join(input)).documentElement
> > d = date.fromtimestamp(os.stat(xml_file).st_mtime)
> >
> > if version == None:
> > @@ -102,13 +111,24 @@ if __name__ == "__main__":
> > else:
> > sys.exit(0)
> >
> > -if len(args) != 1:
> > +if len(args) < 1:
> > sys.stderr.write("%s: exactly 1 non-option arguments required "
> > "(use --help for help)\n" % argv0)
> > sys.exit(1)
> >
> > +input = args[0]
> >
>
>
>
> The 'input' is unused,
>
> Acked-by: Alex Wang
Thanks! I will remove that statement.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v3] ovs-vtep: Support userspace datapaths.
With this commit, the VTEP emulator detects the datapath_type of the
bridge used as a "physical" switch, and creates subsequent bridges
with the same type. This allows ovs-vtep to work with the userspace
datapath.
Signed-off-by: Daniele Di Proietto
---
v2 - v3:
* Use strip('"') to remove the extra quotes
v1 - v2:
Applied Guru's suggestions
* Fixed unspecified datapath_type
* Use more pythonic check for empty string
---
vtep/ovs-vtep | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/vtep/ovs-vtep b/vtep/ovs-vtep
index 60dbb95..a774e02 100755
--- a/vtep/ovs-vtep
+++ b/vtep/ovs-vtep
@@ -40,6 +40,7 @@ vlog = ovs.vlog.Vlog("ovs-vtep")
exiting = False
ps_name = ""
+ps_type = ""
Tunnel_Ip = ""
Lswitches = {}
Bindings = {}
@@ -103,7 +104,12 @@ class Logical_Switch(object):
self.tunnel_key = 0
vlog.warn("invalid tunnel key for %s, using 0" % self.name)
-ovs_vsctl("--may-exist add-br %s" % self.short_name)
+if ps_type:
+ovs_vsctl("--may-exist add-br %s -- set Bridge %s datapath_type=%s"
+ % (self.short_name, self.short_name, ps_type))
+else:
+ovs_vsctl("--may-exist add-br %s" % self.short_name)
+
ovs_vsctl("br-set-external-id %s vtep_logical_switch true"
% self.short_name)
ovs_vsctl("br-set-external-id %s logical_switch_name %s"
@@ -595,6 +601,9 @@ def setup():
if (ps_name not in br_list):
ovs.util.ovs_fatal(0, "couldn't find OVS bridge %s" % ps_name, vlog)
+global ps_type
+ps_type = ovs_vsctl("get Bridge %s datapath_type" % ps_name).strip('"')
+
call_prog("vtep-ctl", ["set", "physical_switch", ps_name,
'description="OVS VTEP Emulator"'])
@@ -636,7 +645,11 @@ def setup():
ovs_vsctl("del-br %s" % br)
-ovs_vsctl("add-br %s" % bfd_bridge)
+if ps_type:
+ovs_vsctl("add-br %s -- set Bridge %s datapath_type=%s"
+ % (bfd_bridge, bfd_bridge, ps_type))
+else:
+ovs_vsctl("add-br %s" % bfd_bridge)
def main():
--
2.1.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] ovs-vtep: Support userspace datapaths.
On 16/06/2015 15:39, "Gurucharan Shetty" wrote:
>
>>
>> +global ps_type
>> +ps_type = ovs_vsctl("get Bridge %s datapath_type" % ps_name)
>> +if ps_type == "\"\"":
>> +ps_type = ""
>Else where in the code, this is handled by calling strip().
>e.g: port_type = ovs_vsctl("get Interface %s type"
> % port).strip('"')
>It sort of looks cleaner to me and provides a little more consistency.
>Otherwise, I am happy with the change.
Makes sense, thanks
I've sent a v3 to the list.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] IGMPv3 support
On Tue, Jun 16, 2015 at 08:16:33AM -0700, Ben Pfaff wrote: > On Mon, Jun 08, 2015 at 01:05:41PM -0300, Thadeu Lima de Souza Cascardo wrote: > > Support IGMPv3 messages with multiple records. Make sure all IGMPv3 > > messages go through slow path, since they may carry multiple multicast > > addresses, unlike IGMPv2. > > > > Tests done: > > > > * multiple addresses in IGMPv3 report are inserted in mdb; > > * address is removed from IGMPv3 if record is INCLUDE_MODE; > > * reports sent on a burst with same flow all go to userspace; > > * IGMPv3 reports go to mrouters, i.e., ports that have issued a query. > > > > Signed-off-by: Thadeu Lima de Souza Cascardo > > Thanks! I have a few comments. > > I am surprised that igmpv3_header in packets.h is marked OVS_PACKED, > because it looks like all of the fields are naturally aligned. Does > this structure need to be packed? > > Similarly for igmpv3_record. However, in this struct I would also > change 'maddr' from ovs_be32 to ovs_16aligned_be32, because there are > situations where packet headers can be aligned on an odd 16-bit boundary > and using ovs_16aligned_be32 makes it easier to handle that safely on > RISC architectures. That was me basing my first version of the code on LACP. I was about to test my v3, I will take the chance and apply your suggestions before I test and resubmit. Thanks. Cascardo. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH ovn 11/11] ovs-sim: New utility for multi-node OVS and OVN simulation.
On Tue, Jun 16, 2015 at 01:07:36AM -0700, Alex Wang wrote:
> Really like this patch~!
Thanks!
> > +
> > +Adds a new port to bridge in the default sandbox (as
> > set
> > +with as) that plugs it into the network
> > +interconnection network. network must already have
> > been
> > +created by a previous invocation of net_add. The
> > default
> > +sandbox must not be main.
> > +
> >
>
>
> First sentence of the description of 'net_attach' is confusing. do you mean
> 's/that/and/'?
That works OK for me too, so I made the change.
> > +for option; do
> > +# This option-parsing mechanism borrowed from a Autoconf-generated
> > +# configure script under the following license:
> > +
> > +# Copyright (C) 1992, 1993, 1994, 1995, 1996, 1998, 1999, 2000, 2001,
> > +# 2002, 2003, 2004, 2005, 2006, 2009, 2013 Free Software Foundation,
> > Inc.
> > +# This configure script is free software; the Free Software Foundation
> > +# gives unlimited permission to copy, distribute and modify it.
> > +
> > +# If the previous option needs an argument, assign it.
> > +if test -n "$prev"; then
> > +eval $prev=\$option
> > +prev=
> > +continue
> > +fi
>
> "prev" not used, i think the above code can be removed?
There's a lot that can go. I removed it all, as well as the copyright
notice since I took out everything that came from Autoconf.
> > +# Check that we've got proper builddir and srcdir.
> > +if test ! -e "$sim_builddir"/vswitchd/ovs-vswitchd; then
> > +echo "$sim_builddir/vswitchd/ovs-vswitchd does not exist (need to run
> > \"make\"?)" >&2
> > +exit 1
> > +fi
> > +if test ! -e "$sim_srcdir"/WHY-OVS.md; then
> > +echo "$sim_srcdir/WHY-OVS.md does not exist" >&2
> > +exit 1
> > +fi
>
> Why WHY-OVS? ;D
The goal is to look for a file that is in the OVS source tree and
unlikely to be in any other source tree, so WHY-OVS seems like a good
choice to me.
> > +# Put built tools early in $PATH.
> > +if test ! -e $sim_builddir/vswitchd/ovs-vswitchd; then
> > +echo >&2 'build not found, please change set $sim_builddir or change
> > directory'
> > +exit 1
> > +fi
>
> Duplicated check?
Thanks, dropped.
> > +PATH=$sim_builddir/ovsdb:$sim_builddir/vswitchd:$sim_builddir/utilities:$PATH
> >
> > +PATH=$sim_builddir/ovn:$sim_srcdir/ovn:$sim_builddir/ovn/controller:$sim_builddir/ovn/northd:$PATH
> > +export PATH
> > +
> > +rm -rf sandbox
> > +mkdir sandbox
> > +cd sandbox
> > +sim_base=`pwd`; export sim_base
> > +trap "cd '$sim_base' && kill \`cat */*.pid\`" 0 1 2 3 13 14 15
> > +
> > +sim_setvars() {
> > +sandbox=$1
> > +OVS_RUNDIR=$sim_base/$1; export OVS_RUNDIR
> > +OVS_LOGDIR=$sim_base/$1; export OVS_LOGDIR
> > +OVS_DBDIR=$sim_base/$1; export OVS_DBDIR
> > +OVS_SYSCONFDIR=$sim_base/$1; export OVS_SYSCONFDIR
> > +PS1="|$1: $sim_PS1"
> > +}
>
> Do you want to add a space between for PS1? (i.e., PS1="| S1: $sim_PS1")
I think it looks OK without the space.
> > +export -f sim_setvars
> > +
> > +as() {
> > +case $# in
> > + 0)
> > +echo >&2 "$FUNCNAME: missing arguments (use --help for help)"
> > +return 1
> > + ;;
> > + 1)
> > + if test "$1" != --help; then
> > + sim_setvars $1
> > + else
> > + cat < > +$FUNCNAME: set the default sandbox for Open vSwitch commands
> > +usage: $FUNCNAME SANDBOX [COMMAND ARG...]
> > +where SANDBOX is the name of the desired sandbox.
> > +
> > +With COMMAND arguments, this command sets the default target for that
> > +single command, which it runs directly. Otherwise, it sets the default
> > +target for all following commands.
> > +EOF
>
> Do you want to prefix all usage prints with '| '? To keep consistency?
I would rather not. It would be a hassle if we ever wanted to change
it, and other commands that the user runs don't have the prefix anyway.
> > +set X $1; shift
> > +if test $# != 1; then
> > +echo >&2 "$FUNCNAME: sandbox name must be a single word"
> > +return 1
> > +fi
> > +
> >
>
>
> This is a cool check~
Thanks.
I think that this is a positive review, so I'm going to push this to
'ovn' in a minute.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v3] ovs-vtep: Support userspace datapaths.
On Tue, Jun 16, 2015 at 8:25 AM, Daniele Di Proietto
wrote:
> With this commit, the VTEP emulator detects the datapath_type of the
> bridge used as a "physical" switch, and creates subsequent bridges
> with the same type. This allows ovs-vtep to work with the userspace
> datapath.
>
> Signed-off-by: Daniele Di Proietto
applied.
> ---
> v2 - v3:
> * Use strip('"') to remove the extra quotes
>
> v1 - v2:
> Applied Guru's suggestions
> * Fixed unspecified datapath_type
> * Use more pythonic check for empty string
> ---
> vtep/ovs-vtep | 17 +++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/vtep/ovs-vtep b/vtep/ovs-vtep
> index 60dbb95..a774e02 100755
> --- a/vtep/ovs-vtep
> +++ b/vtep/ovs-vtep
> @@ -40,6 +40,7 @@ vlog = ovs.vlog.Vlog("ovs-vtep")
> exiting = False
>
> ps_name = ""
> +ps_type = ""
> Tunnel_Ip = ""
> Lswitches = {}
> Bindings = {}
> @@ -103,7 +104,12 @@ class Logical_Switch(object):
> self.tunnel_key = 0
> vlog.warn("invalid tunnel key for %s, using 0" % self.name)
>
> -ovs_vsctl("--may-exist add-br %s" % self.short_name)
> +if ps_type:
> +ovs_vsctl("--may-exist add-br %s -- set Bridge %s
> datapath_type=%s"
> + % (self.short_name, self.short_name, ps_type))
> +else:
> +ovs_vsctl("--may-exist add-br %s" % self.short_name)
> +
> ovs_vsctl("br-set-external-id %s vtep_logical_switch true"
>% self.short_name)
> ovs_vsctl("br-set-external-id %s logical_switch_name %s"
> @@ -595,6 +601,9 @@ def setup():
> if (ps_name not in br_list):
> ovs.util.ovs_fatal(0, "couldn't find OVS bridge %s" % ps_name, vlog)
>
> +global ps_type
> +ps_type = ovs_vsctl("get Bridge %s datapath_type" % ps_name).strip('"')
> +
> call_prog("vtep-ctl", ["set", "physical_switch", ps_name,
> 'description="OVS VTEP Emulator"'])
>
> @@ -636,7 +645,11 @@ def setup():
>
> ovs_vsctl("del-br %s" % br)
>
> -ovs_vsctl("add-br %s" % bfd_bridge)
> +if ps_type:
> +ovs_vsctl("add-br %s -- set Bridge %s datapath_type=%s"
> + % (bfd_bridge, bfd_bridge, ps_type))
> +else:
> +ovs_vsctl("add-br %s" % bfd_bridge)
>
>
> def main():
> --
> 2.1.4
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] IGMPv3 support
On Tue, Jun 16, 2015 at 12:38:06PM -0300, Thadeu Lima de Souza Cascardo wrote: > On Tue, Jun 16, 2015 at 08:16:33AM -0700, Ben Pfaff wrote: > > On Mon, Jun 08, 2015 at 01:05:41PM -0300, Thadeu Lima de Souza Cascardo > > wrote: > > > Support IGMPv3 messages with multiple records. Make sure all IGMPv3 > > > messages go through slow path, since they may carry multiple multicast > > > addresses, unlike IGMPv2. > > > > > > Tests done: > > > > > > * multiple addresses in IGMPv3 report are inserted in mdb; > > > * address is removed from IGMPv3 if record is INCLUDE_MODE; > > > * reports sent on a burst with same flow all go to userspace; > > > * IGMPv3 reports go to mrouters, i.e., ports that have issued a query. > > > > > > Signed-off-by: Thadeu Lima de Souza Cascardo > > > > Thanks! I have a few comments. > > > > I am surprised that igmpv3_header in packets.h is marked OVS_PACKED, > > because it looks like all of the fields are naturally aligned. Does > > this structure need to be packed? > > > > Similarly for igmpv3_record. However, in this struct I would also > > change 'maddr' from ovs_be32 to ovs_16aligned_be32, because there are > > situations where packet headers can be aligned on an odd 16-bit boundary > > and using ovs_16aligned_be32 makes it easier to handle that safely on > > RISC architectures. > > That was me basing my first version of the code on LACP. I was about to > test my v3, I will take the chance and apply your suggestions before I > test and resubmit. Thanks for the hint about LACP. It looks like one of the structures there doesn't really need to be packed either. I'll send a patch. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] lacp: Remove packed attribute from struct lacp_pdu.
The packed annotation doesn't do anything here because all of the members
in the structure are naturally aligned.
Signed-off-by: Ben Pfaff
---
lib/lacp.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/lib/lacp.c b/lib/lacp.c
index 65149fd..29b8b43 100644
--- a/lib/lacp.c
+++ b/lib/lacp.c
@@ -62,7 +62,6 @@ struct lacp_info {
BUILD_ASSERT_DECL(LACP_INFO_LEN == sizeof(struct lacp_info));
#define LACP_PDU_LEN 110
-OVS_PACKED(
struct lacp_pdu {
uint8_t subtype; /* Always 1. */
uint8_t version; /* Always 1. */
@@ -81,7 +80,7 @@ struct lacp_pdu {
uint8_t collector_len;/* Always 16. */
ovs_be16 collector_delay; /* Maximum collector delay. Set to UINT16_MAX. */
uint8_t z3[64]; /* Combination of several fields. Always 0. */
-});
+};
BUILD_ASSERT_DECL(LACP_PDU_LEN == sizeof(struct lacp_pdu));
/* Implementation. */
--
2.1.3
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] datapath-windows: BSOD when disabling the extension
On Mon, Jun 15, 2015 at 02:49:04PM +, Sorin Vinturis wrote: > When the filter detach routine is called while there are packets > still in processing, the OvsUninitSwitchContext function call will > decrement the switch context reference count without releasing the > switch context structure. This behaviour is correct and expected, > but the BSOD is caused in this case because the gOvsSwitchContext > variable is set to NULL, which is wrong. > > The gOvsSwitchContext global variable must be set to NULL only when > the switch context structure is actually released. > > Signed-off-by: Sorin Vinturis > Reported-by: Sorin Vinturis > Reported-at: https://github.com/openvswitch/ovs-issues/issues/80 > Acked-by: Alin Gabriel Serdean It appears that I applied this (or an earlier version?) yesterday. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH ovn v2] ofctrl: Don't use designated initializers.
On Tue, Jun 16, 2015 at 06:44:35AM -0700, Gurucharan Shetty wrote: > MSVC 2013 does not like designated initializers when > structs are initialized inside structs. > Apparently it has been fixed in MSVC 2015. > > Signed-off-by: Gurucharan Shetty Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH ovn 11/11] ovs-sim: New utility for multi-node OVS and OVN simulation.
> > > > +set X $1; shift > > > +if test $# != 1; then > > > +echo >&2 "$FUNCNAME: sandbox name must be a single word" > > > +return 1 > > > +fi > > > + > > > > > > > > > This is a cool check~ > > Thanks. > > I think that this is a positive review, so I'm going to push this to > 'ovn' in a minute. > Sounds good, Acked-by: Alex Wang ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] dpif-netdev: Prefetch next packet before miniflow_extract().
On 16/06/2015 09:55, "Gray, Mark D" wrote: >> > >> Acked-by: Ethan Jackson > >> > >> One question I had for a future patch. Have you considered prefetching > >> more packets at once? I.E. 4 at a time or something? > >> That's how these things are typically written, though I don't know if >>it would > >> actually affect things. > > > >yeah, I was going to suggest the same thing. In most DPDK apps, we would > >prefetch about 3. I've very briefly trying to do something more sophisticated, but I didn't manage to get any significant improvement. I just posted this because it's a simple 5 lines change that removes an easy bottleneck. I definitely agree that it might be worth to work more on that code in the future to squeeze more pps. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] Is this an issue for DPDK vhost rss?
On Mon, Jun 15, 2015 at 05:55:13PM +, Daniele Di Proietto wrote: > On 15/06/2015 12:16, "Traynor, Kevin" wrote: > >There is a dpdk patchset that contains a potential fix for this and lots > >of > >other changes, but I haven't tested yet. > >https://urldefense.proofpoint.com/v2/url?u=http-3A__dpdk.org_ml_archives_d > >ev_2015-2DJune_018436.html&d=BQIFAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMN > >tXt-uEs&r=SmB5nZacmXNq0gKCC1s_Cw5yUNjxgD4v5kJqZ2uWLlE&m=FDVPKa2SqwpyYOTmA2 > >zGdscCPa1FVdQG3Zbr4tHrp38&s=fjg7wArWvYLJlgEGKijK6W6ECAxGk660UrPF3rAr4Rs&e= I skimmed over the patchset and it is an ABI breaker, so I think the policy demands to announce it on 2.1 and merge only in 2.2 release. Maybe it is possible to separate the ol_flags fix into a smaller and simple patch to be accepted as bugfix yet in 2.1. > >> > Otherwise, should we avoid using the vectorized version? > >> > >> that's debatable - from a performance view it may be better to leave it > >>in > >> and take the hit elsewhere for the time being if there's a possibility > >>that > >> it will be changed in DPDK later. > > > >With a loop to reset the rss after the rte_vhost_dequeue_burst() call I'm > >seeing a drop of ~100kpps in vhost performance. Rx vectoristion gives a > >gain > >of about ~1 mpps on my system for the phy2phy cases. > > > >Using the ol_flags check is the right option when DPDK supports setting it > >correctly with rx vectorisation. In the meantime there's choice of using > >the > >reset loop or removing rx vectorisation - what do you think? > > Thanks for sharing these results. I've observed that if OVS can't use the > RSS > hash and has to compute we lose ~2Mpps on a single flow phy2phy test. > > Despite this, I still think we should consider the ol_flags because: > > * DPDK drivers (other than ixgbe) should use ol_flags as well to mark the > RSS hash as valid > * ixgbe_recv_pkts_vec() will report PKT_RX_RSS_HASH in future releases (the > patch you sent will be effective since DPDK 2.2, right?) I agree with the above. > If the throughput with the non-vector rx routine is higher we can disable > the vector rx as a temporary workaround. Could you point me to the vector and non-vector rx routines? I feel like I am missing something. Thanks, fbl ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH ovn] ovn: Remove completed items from TODO.
Signed-off-by: Ben Pfaff --- ovn/TODO | 85 1 file changed, 85 deletions(-) diff --git a/ovn/TODO b/ovn/TODO index fe296b4..07d66da 100644 --- a/ovn/TODO +++ b/ovn/TODO @@ -1,64 +1,5 @@ * ovn-controller -** Flow table handling in ovn-controller. - - ovn-controller has to transform logical datapath flows from the - database into OpenFlow flows. - -*** Definition (or choice) of data structure for flows and flow table. - -It would be natural enough to use "struct flow" and "struct -classifier" for this. Maybe that is what we should do. However, -"struct classifier" is optimized for searches based on packet -headers, whereas all we care about here can be implemented with a -hash table. Also, we may want to make it easy to add and remove -support for fields without recompiling, which is not possible with -"struct flow" or "struct classifier". - -On the other hand, we may find that it is difficult to decide that -two OXM flow matches are identical (to normalize them) without a -lot of domain-specific knowledge that is already embedded in struct -flow. It's also going to be a pain to come up with a way to make -anything other than "struct flow" work with the ofputil_*() -functions for encoding and decoding OpenFlow. - -It's also possible we could use struct flow without struct -classifier. - -*** Translating logical datapath actions into OpenFlow actions. - -Some of the logical datapath actions do not have natural -representations as OpenFlow actions: they require -packet-in/packet-out round trips through ovn-controller. The -trickiest part of that is going to be making sure that the -packet-out resumes the control flow that was broken off by the -packet-in. That's tricky; we'll probably have to restrict control -flow or add OVS features to make resuming in general possible. Not -sure which is better at this point. - -*** OpenFlow flow table synchronization. - -The internal representation of the OpenFlow flow table has to be -synced across the controller connection to OVS. This probably -boils down to the "flow monitoring" feature of OF1.4 which was then -made available as a "standard extension" to OF1.3. (OVS hasn't -implemented this for OF1.4 yet, but the feature is based on a OVS -extension to OF1.0, so it should be straightforward to add it.) - -We probably need some way to catch cases where OVS and OVN don't -see eye-to-eye on what exactly constitutes a flow, so that OVN -doesn't waste a lot of CPU time hammering at OVS trying to install -something that it's not going to do. - -*** Logical/physical translation. - -When a packet comes into the integration bridge, the first stage of -processing needs to translate it from a physical to a logical -context. When a packet leaves the integration bridge, the final -stage of processing needs to translate it back into a physical -context. ovn-controller needs to populate the OpenFlow flows -tables to do these translations. - *** Determine how to split logical pipeline across physical nodes. From the original OVN architecture document: @@ -78,8 +19,6 @@ The split pipeline processing split will influence how tunnel keys are encoded. -*** Monitor Pipeline table in OVN, trigger flow table recomputation on change. - ** ovn-controller parameters and configuration. *** SSL configuration. @@ -94,13 +33,6 @@ Andy Zhou is looking at these issues. -** Scaling number of connections. - - In typical use today a given ovsdb-server has only a single-digit - number of simultaneous connections. The OVN Southbound database will - have a connection from every hypervisor. This use case needs testing - and probably coding work. Here are some possible improvements. - *** Reducing amount of data sent to clients. Currently, whenever a row monitored by a client changes, @@ -161,20 +93,3 @@ Epstein et al., "What's the Difference? Efficient Set Reconciliation Without Prior Context". (I'm not yet aware of previous non-academic use of this technique.) - -* Miscellaneous: - -** Init scripts for ovn-controller (on HVs), ovn-northd, OVN DB server. - -** Distribution packaging. - -* Not yet scoped: - -** Neutron plugin. - - This is being developed on OpenStack's development infrastructure - to be along side most of the other Neutron plugins. - - http://git.openstack.org/cgit/openstack/networking-ovn - -** Gateways. -- 2.1.3 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] fix wrong cmd for create dpdkvhostuser in INSTALL.DPDK.md
On Tue, Jun 16, 2015 at 11:23:07AM +0530, gowrishankar wrote: > Also updated an additional correction as in : > http://openvswitch.org/pipermail/dev/2015-June/056379.html > > Could you include that as well. Yeap. One patch with all the s/ovs-vsctl/ovs-vsctl/ and another for the scheme initialization? Thanks, fbl ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] Do not flush tx queue which is shared among CPUs since it is always flushed
On 16/06/2015 07:40, "Pravin Shelar" wrote:
>On Mon, Jun 8, 2015 at 7:42 PM, Pravin Shelar wrote:
>> On Mon, Jun 8, 2015 at 6:13 PM, Wei li wrote:
>>> When tx queue is shared among CPUS,the pkts always be flush in
>>>'netdev_dpdk_eth_send'
>>> So it is unnecessarily for flushing in netdev_dpdk_rxq_recv
>>> Otherwise tx will be accessed without locking
>>>
>>> Signed-off-by: Wei li
>>> ---
>>> v1->v2: fix typo
>>>
>>> lib/netdev-dpdk.c | 7 +--
>>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>>> index 63243d8..1e0a483 100644
>>> --- a/lib/netdev-dpdk.c
>>> +++ b/lib/netdev-dpdk.c
>>> @@ -892,8 +892,11 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq_,
>>>struct dp_packet **packets,
>>> int nb_rx;
>>>
>>> /* There is only one tx queue for this core. Do not flush other
>>> - * queueus. */
>>> -if (rxq_->queue_id == rte_lcore_id()) {
>>> + * queues.
>>> + * Do not flush tx queue which is shared among CPUs
>>> + * since it is always flushed */
>>> +if (rxq_->queue_id == rte_lcore_id() &&
>>> + OVS_LIKELY(!dev->txq_needs_locking)) {
>>> dpdk_queue_flush(dev, rxq_->queue_id);
>>> }
>>
>> Patch looks good, But Daniele has plan to get rid of queue flushing
>> logic. do you see problem with doing this?
>
>Daniele,
>I am not sure if we should fix this queue flushing logic if we are
>going to remove it. So I would like to discuss it first.
>You mentioned that the removing flushing logic results in performance
>drop. Can you explain it? How much is performance drop and which is
>the case?
Hi Pravin,
sorry for the late reply. I suspected that removing the queues in
netdev-dpdk
was going to have a performance impact in the following scenario: a batch
of
packets from different megaflows with the same action (output on port 1).
Here's what happens:
1) With the queues in netdev-dpdk. dpif-netdev calls netdev_send() for
each
packet. netdev_dpdk_send() just copies the pointer in the queue. Before
receiving the next batch dpdk_queue_flush() call rte_eth_tx_burst() with
a whole batch of packets
2) Without the queues in netdev-dpdk. dpif-netdev calls netdev_send() for
each
packet. netdev_dpdk_send() calls rte_eth_tx_burst() for each packet.
Scenario 2 could be slower because rte_eth_tx_burst() is called for each
packet
(instead of each batch). After some testing this turns out not to be the
case.
It appears that the bottleneck is not rte_eth_tx_burst(), and copying the
pointers
in the temporary queue makes the code slower.
What do you think? Should we just remove the queues?
CC'ing Mark since he expressed interest in the issue
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] dpif-netdev: Prefetch next packet before miniflow_extract().
If you tried it and it didn't improve, then I think what we have now is great. Ethan On Tue, Jun 16, 2015 at 9:08 AM, Daniele Di Proietto wrote: > > On 16/06/2015 09:55, "Gray, Mark D" wrote: > >>> >> >>> Acked-by: Ethan Jackson >> >>> >> >>> One question I had for a future patch. Have you considered prefetching >> >>> more packets at once? I.E. 4 at a time or something? >> >>> That's how these things are typically written, though I don't know if >>>it would >> >>> actually affect things. >> >> >> >>yeah, I was going to suggest the same thing. In most DPDK apps, we would >> >>prefetch about 3. > > I've very briefly trying to do something more sophisticated, but I didn't > manage to get any significant improvement. > > I just posted this because it's a simple 5 lines change that removes an > easy > bottleneck. > > I definitely agree that it might be worth to work more on that code in > the future to squeeze more pps. > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] lacp: Remove packed attribute from struct lacp_pdu.
On Tue, Jun 16, 2015 at 08:47:34AM -0700, Ben Pfaff wrote: > The packed annotation doesn't do anything here because all of the members > in the structure are naturally aligned. > > Signed-off-by: Ben Pfaff > --- Acked-by: Flavio Leitner ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH ovn 2/2 v5] fedora.spec: Create openvswitch-ovn package.
On Mon, Jun 15, 2015 at 02:31:45PM -0700, Ben Pfaff wrote: > On Fri, Jun 12, 2015 at 12:51:24PM -0400, Russell Bryant wrote: > > This patch creates a new subpackage for OVN, openvswitch-ovn. It also > > installs systemd unit files for ovn-controller and ovn-northd. > > > > If you want to run ovn-controller: > > > > # systemctl start ovn-controller > > > > If you want to run ovn-northd: > > > > # systemctl start ovn-northd > > > > Both systemd units are currently set to depend on openvswitch. If > > further ovsdb initialization is required for the OVN databases before > > ovn-northd can start, that will be handled automatically by ovn-ctl > > when you start the ovn-northd service. > > > > This currently assumes that ovn-northd runs on the same host as > > ovsdb-server that is hosting the OVN databases. That seems like a > > reasonable assumption in the current architecture and can be evolved > > later when needed. > > > > Signed-off-by: Russell Bryant > > CC: Flavio Leitner > > CC: Ben Pfaff > > Applied to 'ovn', thanks! Nice! > (I only skimmed this, on the basis that you know RPM packaging better > than me.) It LGTM as well. Thanks folks, fbl ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [RFC V2 4/4] ofprot-dpif-hsa: Implement HSA prototype.
Sorry for this very delayed reply, thx so much for the review~! Addressed all comments to this series, I think I have three more things to do here: 1. adding more unit tests, 2. making this command run in a separate thread, 3. received a request to add another command to detect unused openflow flows (and test on the given flows) I'll address them and post the final~ patches~ Thanks, Alex Wang, On Sun, Jun 7, 2015 at 9:58 AM, Ben Pfaff wrote: > On Mon, Jun 01, 2015 at 11:24:19AM -0700, Alex Wang wrote: > > This commit implements a prototype of Header Space Analysis of the > > OVS OpenFlow table. > > > > What It Does > > > > > > 1. Dump all OpenFlow rules from a specified bridge. > > 2. Generate all-unmasked header space with specified input port number > > and start looking up from default starting table. > > 3. Traverse through all possible processing pipelines (i.e. OpenFlow > > rules cascaded with resubmit action) and conduct leak check or loop > > check. > > 3.1 A leak check prints all possible output ports with the actual input > > header space format. > > 3.2 A loop check checks if an OpenFlow rule is matched multiple time in > > the processing pipeline. It will tell if the loop is finite or > > infinite and print the loop path. > > > > How To Use > > -- > > > > ovs-appctl hsa/detect-leak > > ovs-appctl hsa/detect-loop > > > > Limitation > > -- > > > > - Only support a very limited set of OFPACT_* actions. And extending > > to accept more actions can be very complicated. > > - Single thread implementation, can take 20+ mins to run on a > production > > setup with ~100 OpenFlow rules. > > > > Signed-off-by: Alex Wang > > > > --- > > RFC->RFC V2: > > - Clarify comments of 'move_map'. > > - Change the 'move_map' to 2-D array. > > - Make hs_clone do not double init (but seems to make it more complex). > > - Fix the overflow risk in hsa_rule_compare(). > > - Adopt Ben's suggestions. > > - Add example unit test (did not add complete tests since I think we may > > want to remodel it for OVN). > > I would add documentation for this (roughly what is in the commit > message above) to ovs-vswitchd.8.in, in the section dedicated to > ovs-appctl commands. > > I think that this is also worth mentioning in NEWS. > > Acked-by: Ben Pfaff > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] lacp: Remove packed attribute from struct lacp_pdu.
Acked-by: Ethan Jackson On Tue, Jun 16, 2015 at 10:53 AM, Flavio Leitner wrote: > On Tue, Jun 16, 2015 at 08:47:34AM -0700, Ben Pfaff wrote: >> The packed annotation doesn't do anything here because all of the members >> in the structure are naturally aligned. >> >> Signed-off-by: Ben Pfaff >> --- > > Acked-by: Flavio Leitner > > > ___ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH ovn] ovn: Remove completed items from TODO.
Good to see these done!~ Acked-by: Alex Wang On Tue, Jun 16, 2015 at 10:33 AM, Ben Pfaff wrote: > Signed-off-by: Ben Pfaff > --- > ovn/TODO | 85 > > 1 file changed, 85 deletions(-) > > diff --git a/ovn/TODO b/ovn/TODO > index fe296b4..07d66da 100644 > --- a/ovn/TODO > +++ b/ovn/TODO > @@ -1,64 +1,5 @@ > * ovn-controller > > -** Flow table handling in ovn-controller. > - > - ovn-controller has to transform logical datapath flows from the > - database into OpenFlow flows. > - > -*** Definition (or choice) of data structure for flows and flow table. > - > -It would be natural enough to use "struct flow" and "struct > -classifier" for this. Maybe that is what we should do. However, > -"struct classifier" is optimized for searches based on packet > -headers, whereas all we care about here can be implemented with a > -hash table. Also, we may want to make it easy to add and remove > -support for fields without recompiling, which is not possible with > -"struct flow" or "struct classifier". > - > -On the other hand, we may find that it is difficult to decide that > -two OXM flow matches are identical (to normalize them) without a > -lot of domain-specific knowledge that is already embedded in struct > -flow. It's also going to be a pain to come up with a way to make > -anything other than "struct flow" work with the ofputil_*() > -functions for encoding and decoding OpenFlow. > - > -It's also possible we could use struct flow without struct > -classifier. > - > -*** Translating logical datapath actions into OpenFlow actions. > - > -Some of the logical datapath actions do not have natural > -representations as OpenFlow actions: they require > -packet-in/packet-out round trips through ovn-controller. The > -trickiest part of that is going to be making sure that the > -packet-out resumes the control flow that was broken off by the > -packet-in. That's tricky; we'll probably have to restrict control > -flow or add OVS features to make resuming in general possible. Not > -sure which is better at this point. > - > -*** OpenFlow flow table synchronization. > - > -The internal representation of the OpenFlow flow table has to be > -synced across the controller connection to OVS. This probably > -boils down to the "flow monitoring" feature of OF1.4 which was then > -made available as a "standard extension" to OF1.3. (OVS hasn't > -implemented this for OF1.4 yet, but the feature is based on a OVS > -extension to OF1.0, so it should be straightforward to add it.) > - > -We probably need some way to catch cases where OVS and OVN don't > -see eye-to-eye on what exactly constitutes a flow, so that OVN > -doesn't waste a lot of CPU time hammering at OVS trying to install > -something that it's not going to do. > - > -*** Logical/physical translation. > - > -When a packet comes into the integration bridge, the first stage of > -processing needs to translate it from a physical to a logical > -context. When a packet leaves the integration bridge, the final > -stage of processing needs to translate it back into a physical > -context. ovn-controller needs to populate the OpenFlow flows > -tables to do these translations. > - > *** Determine how to split logical pipeline across physical nodes. > > From the original OVN architecture document: > @@ -78,8 +19,6 @@ > The split pipeline processing split will influence how tunnel keys > are encoded. > > -*** Monitor Pipeline table in OVN, trigger flow table recomputation on > change. > - > ** ovn-controller parameters and configuration. > > *** SSL configuration. > @@ -94,13 +33,6 @@ > >Andy Zhou is looking at these issues. > > -** Scaling number of connections. > - > - In typical use today a given ovsdb-server has only a single-digit > - number of simultaneous connections. The OVN Southbound database will > - have a connection from every hypervisor. This use case needs testing > - and probably coding work. Here are some possible improvements. > - > *** Reducing amount of data sent to clients. > > Currently, whenever a row monitored by a client changes, > @@ -161,20 +93,3 @@ > Epstein et al., "What's the Difference? Efficient Set > Reconciliation Without Prior Context". (I'm not yet aware of > previous non-academic use of this technique.) > - > -* Miscellaneous: > - > -** Init scripts for ovn-controller (on HVs), ovn-northd, OVN DB server. > - > -** Distribution packaging. > - > -* Not yet scoped: > - > -** Neutron plugin. > - > - This is being developed on OpenStack's development infrastructure > - to be along side most of the other Neutron plugins. > - > - http://git.openstack.org/cgit/openstack/networking-ovn > - > -** Gateways. > -- > 2.1.3 > > _
[ovs-dev] [PATCH] dpif-netdev: Check for PKT_RX_RSS_HASH flag.
DPDK mbufs contain a valid RSS hash only if PKT_RX_RSS_HASH is
set in 'ol_flags'. Otherwise the hash is garbage and doesn't
relate to the packet.
This fixes an issue with vhost, which, being a virtual NIC, doesn't
compute the hash.
Unfortunately the ixgbe vPMD doesn't set the PKT_RX_RSS_HASH, forcing
OVS to compute an hash is software. This has a significant impact on
performance (-30% throughput in a single flow setup) which can be
mitigated in the CPU supports crc32c instructions.
Reported-by: Dongjun
Suggested-by: Flavio Leitner
Signed-off-by: Daniele Di Proietto
---
lib/dp-packet.h | 11 +++
lib/dpif-netdev.c | 2 +-
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index e4c2593..6840750 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -529,11 +529,22 @@ dp_packet_set_rss_hash(struct dp_packet *p, uint32_t hash)
{
#ifdef DPDK_NETDEV
p->mbuf.hash.rss = hash;
+p->mbuf.ol_flags |= PKT_RX_RSS_HASH;
#else
p->rss_hash = hash;
#endif
}
+static inline bool
+dp_packet_rss_valid(struct dp_packet *p)
+{
+#ifdef DPDK_NETDEV
+return p->mbuf.ol_flags & PKT_RX_RSS_HASH;
+#else
+return true;
+#endif
+}
+
#ifdef __cplusplus
}
#endif
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index f13169c..c4a4b3a 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -3036,7 +3036,7 @@ dpif_netdev_packet_get_rss_hash(struct dp_packet *packet,
{
uint32_t hash, recirc_depth;
-hash = dp_packet_get_rss_hash(packet);
+hash = dp_packet_rss_valid(packet) ? dp_packet_get_rss_hash(packet) : 0;
if (OVS_UNLIKELY(!hash)) {
hash = miniflow_hash_5tuple(mf, 0);
dp_packet_set_rss_hash(packet, hash);
--
2.1.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] Is this an issue for DPDK vhost rss?
On 16/06/2015 18:27, "Flavio Leitner" wrote: >On Mon, Jun 15, 2015 at 05:55:13PM +, Daniele Di Proietto wrote: >> On 15/06/2015 12:16, "Traynor, Kevin" wrote: >> >There is a dpdk patchset that contains a potential fix for this and >>lots >> >of >> >other changes, but I haven't tested yet. >> >>>https://urldefense.proofpoint.com/v2/url?u=http-3A__dpdk.org_ml_archives >>>_d >> >>>ev_2015-2DJune_018436.html&d=BQIFAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihV >>>MN >> >>>tXt-uEs&r=SmB5nZacmXNq0gKCC1s_Cw5yUNjxgD4v5kJqZ2uWLlE&m=FDVPKa2SqwpyYOTm >>>A2 >> >>>zGdscCPa1FVdQG3Zbr4tHrp38&s=fjg7wArWvYLJlgEGKijK6W6ECAxGk660UrPF3rAr4Rs& >>>e= > >I skimmed over the patchset and it is an ABI breaker, so I >think the policy demands to announce it on 2.1 and merge only >in 2.2 release. > >Maybe it is possible to separate the ol_flags fix into a >smaller and simple patch to be accepted as bugfix yet in 2.1. That'd be ideal. > >> >> > Otherwise, should we avoid using the vectorized version? >> >> >> >> that's debatable - from a performance view it may be better to leave >>it >> >>in >> >> and take the hit elsewhere for the time being if there's a >>possibility >> >>that >> >> it will be changed in DPDK later. >> > >> >With a loop to reset the rss after the rte_vhost_dequeue_burst() call >>I'm >> >seeing a drop of ~100kpps in vhost performance. Rx vectoristion gives a >> >gain >> >of about ~1 mpps on my system for the phy2phy cases. >> > >> >Using the ol_flags check is the right option when DPDK supports >>setting it >> >correctly with rx vectorisation. In the meantime there's choice of >>using >> >the >> >reset loop or removing rx vectorisation - what do you think? >> >> Thanks for sharing these results. I've observed that if OVS can't use >>the >> RSS >> hash and has to compute we lose ~2Mpps on a single flow phy2phy test. >> >> Despite this, I still think we should consider the ol_flags because: >> >> * DPDK drivers (other than ixgbe) should use ol_flags as well to mark >>the >> RSS hash as valid >> * ixgbe_recv_pkts_vec() will report PKT_RX_RSS_HASH in future releases >>(the >> patch you sent will be effective since DPDK 2.2, right?) > >I agree with the above. I just sent out a patch that uses the approach you suggested. I didn't modify dp_packet_get_rss_hash(), because other code relies on that to return a proper value (computed by the hardware or the software). > >> If the throughput with the non-vector rx routine is higher we can >>disable >> the vector rx as a temporary workaround. > >Could you point me to the vector and non-vector rx routines? >I feel like I am missing something. ixgbe_recv_pkts_vec() in drivers/net/ixgbe/ixgbe_rxtx.c is the vector rx. There are multiple version of the non-vector rx routine, I believe we used ixgbe_recv_pkts() in drivers/net/ixgbe/ixgbe_rxtx.c Thanks > >Thanks, >fbl > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] conntrack: nfqueue action
On 06/12/15 20:31, Ansis Atteka wrote: On Fri, Jun 12, 2015 at 5:50 AM, Franck Baudin wrote: Hi Ansis, On 06/09/15 22:59, Ansis Atteka wrote: Hi Franck On 8 June 2015 at 09:34, Franck BAUDIN mailto:franck.bau...@qosmos.com>> wrote: Hello, Conntrack looks in very good progress on https://github.com/justinpettit/ovs.git However, I didn't find any code related to "nfqueue" openvswitch action, neither on https://github.com/tgraf/ovs.git. Is the nfqueue action still planned to be implemented for openvswitch 2.4? Do you need a hand on this topic? Unfortunately, I am not aware of anyone working actively on this. There are some difficulties that we see with implementing NFQueue verdicts properly so that packet processing could be resumed. If you have design proposal on how to solve this, then I would be glad to hear your opinion. Also, do you think that Open vSwitch kernel module's userspace() action might somehow suffice your use cases so that user-space process would be able to get packet from kernel-space? Unfortunately, userspace() is not an option as DPI is not embedded in ovs-vswitchd. DPI is a standalone userland process. While I haven't tried it myself, It is possible for process other than ovs-vswitchd to get packets sent from OVS kernel module to userspace via userspace() action by making your process to subscribe to the right OVS NETLINK notifications. Of course we would still have to figure out how to implement corresponding OpenFlow DPI action that would translate into this Kernel userspace(dpi) action. Thanks for the tip! I need to dig a bit to see if this would require a datapath patch, an to figure out if this way would perform better than tap. However, I found another way which is pretty straightforward: I can use a dedicated vswitch port to get a copy of the traffic I want to analyze with the DPI. I just need one port per conntrack zone. Yes, it must be somehow possible to be able to pass "conntrack zone" context to the DPI engine. However, are you implying here that you would create Interfaces with type="internal" per conntrack zone? Yes, I need to find a way to get the conntrack zone for every packet, so having a port per zone is an option. I can also use an encapsulation to encode the conntrack zone (vlan for instance) if there are numerous conntrack zones. I used the following rules successfully to send the packets 'under DPI classification' (conn_mark=0 in this example, I'm gonna try conn_label as well as both should work) to the LOCAL port, and when I mark it at some point (conntrack -U -s 1.1.1.1 -p tcp --sport 37120 -m 8), I see that the packet is properly forwarded and no more copied to LOCAL: ovs-ofctl del-flows br0 ovs-ofctl add-flow br0 'priority=5,tcp,conn_state=-trk,action=ct(commit,recirc)' ovs-ofctl add-flow br0 'priority=36000,tcp,conn_state=+trk,conn_mark=8,action=mod_nw_tos:8,NORMAL' ovs-ofctl add-flow br0 'priority=35000,tcp,conn_state=+trk,action=mod_nw_tos:4,LOCAL,NORMAL' ovs-ofctl add-flow br0 'action=NORMAL' Just to say that with Justin's current code, we have everything to implement DPI/L7 classification on openvswitch. Then, we'll see if optimizations/enhancements like NFQUEUE are relevant. Or did I miss something and this design doesn't fly? Best Regards Franck ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] dpif-netdev: Check for PKT_RX_RSS_HASH flag.
On Tue, Jun 16, 2015 at 11:39 AM, Daniele Di Proietto
wrote:
> DPDK mbufs contain a valid RSS hash only if PKT_RX_RSS_HASH is
> set in 'ol_flags'. Otherwise the hash is garbage and doesn't
> relate to the packet.
>
> This fixes an issue with vhost, which, being a virtual NIC, doesn't
> compute the hash.
>
> Unfortunately the ixgbe vPMD doesn't set the PKT_RX_RSS_HASH, forcing
> OVS to compute an hash is software. This has a significant impact on
> performance (-30% throughput in a single flow setup) which can be
> mitigated in the CPU supports crc32c instructions.
>
> Reported-by: Dongjun
> Suggested-by: Flavio Leitner
> Signed-off-by: Daniele Di Proietto
> ---
> lib/dp-packet.h | 11 +++
> lib/dpif-netdev.c | 2 +-
> 2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> index e4c2593..6840750 100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -529,11 +529,22 @@ dp_packet_set_rss_hash(struct dp_packet *p, uint32_t
> hash)
> {
> #ifdef DPDK_NETDEV
> p->mbuf.hash.rss = hash;
> +p->mbuf.ol_flags |= PKT_RX_RSS_HASH;
> #else
> p->rss_hash = hash;
> #endif
> }
>
> +static inline bool
> +dp_packet_rss_valid(struct dp_packet *p)
> +{
> +#ifdef DPDK_NETDEV
> +return p->mbuf.ol_flags & PKT_RX_RSS_HASH;
> +#else
> +return true;
> +#endif
> +}
> +
> #ifdef __cplusplus
> }
> #endif
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index f13169c..c4a4b3a 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -3036,7 +3036,7 @@ dpif_netdev_packet_get_rss_hash(struct dp_packet
> *packet,
> {
> uint32_t hash, recirc_depth;
>
> -hash = dp_packet_get_rss_hash(packet);
> +hash = dp_packet_rss_valid(packet) ? dp_packet_get_rss_hash(packet) : 0;
> if (OVS_UNLIKELY(!hash)) {
We can just check for PKT_RX_RSS_HASH, it saves one check for zero
hash. Is zero hash not valid hash value?
> hash = miniflow_hash_5tuple(mf, 0);
> dp_packet_set_rss_hash(packet, hash);
> --
> 2.1.4
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH/RFC net-next] openvswitch: allow output of MPLS packets on tunnel vports
On Mon, Jun 15, 2015 at 5:39 PM, Simon Horman wrote: > Currently output of MPLS packets on tunnel vports is not allowed by the > datapath and, moreover, flows that match on MPLS packets and output to > tunnel vports are rejected by the datapath. The flows are rejected > regardless of if they also output to non-tunnel vports which is allowed for > MPLS packets and the following is logged by the kernel. > > openvswitch: netlink: Flow actions may not be safe on all matching packets. > > This patch addresses the above by allowing output of MPLS packets to tunnel > vports. > > My recollection of adding MPLS support to the datapath was that a rather > conservative approach was taken in order to minimise the chance of fallout. > This patch proposes relaxing one restriction which was introduced at that > time. > > My limited testing has not isolated any side effects of this change. > > Signed-off-by: Simon Horman > --- > net/openvswitch/flow_netlink.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c > index 624e41c4267f..a5d3c0ae8ac8 100644 > --- a/net/openvswitch/flow_netlink.c > +++ b/net/openvswitch/flow_netlink.c > @@ -1847,9 +1847,6 @@ static int validate_set(const struct nlattr *a, > break; > > case OVS_KEY_ATTR_TUNNEL: > - if (eth_p_mpls(eth_type)) > - return -EINVAL; > - One of the problem is with setting skb->inner_protocol. MPLS and tunnel both needs to set inner protocol field. So outer encapsulation would just overwrite earlier inner protocol field on packet transit path. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 1/2] Make IGMP packets always take slow path
IGMP packets need to take the slow path. Otherwise, packets that match
the same flow will not be processed by OVS. That might prevent OVS from
updating the expire time for entries already in the mdb, but also to
lose packets with different addresses in the payload.
Signed-off-by: Thadeu Lima de Souza Cascardo
---
ofproto/ofproto-dpif-xlate.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index f5dc272..0b113a2 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2269,12 +2269,18 @@ xlate_normal(struct xlate_ctx *ctx)
struct mcast_group *grp;
if (flow->nw_proto == IPPROTO_IGMP) {
-if (ctx->xin->may_learn) {
-if (mcast_snooping_is_membership(flow->tp_src) ||
-mcast_snooping_is_query(flow->tp_src)) {
+if (mcast_snooping_is_membership(flow->tp_src) ||
+mcast_snooping_is_query(flow->tp_src)) {
+if (ctx->xin->may_learn) {
update_mcast_snooping_table(ctx->xbridge, flow, vlan,
in_xbundle);
-}
+}
+/*
+ * IGMP packets need to take the slow path, in order to be
+ * processed for mdb updates. That will prevent expires
+* firing off even after hosts have sent reports.
+*/
+ctx->xout->slow |= SLOW_ACTION;
}
if (mcast_snooping_is_membership(flow->tp_src)) {
--
2.4.2
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 2/2] IGMPv3 support
Support IGMPv3 messages with multiple records. Make sure all IGMPv3
messages go through slow path, since they may carry multiple multicast
addresses, unlike IGMPv2.
Tests done:
* multiple addresses in IGMPv3 report are inserted in mdb;
* address is removed from IGMPv3 if record is INCLUDE_MODE;
* reports sent on a burst with same flow all go to userspace;
* IGMPv3 reports go to mrouters, i.e., ports that have issued a query.
Signed-off-by: Thadeu Lima de Souza Cascardo
---
lib/mcast-snooping.c | 52
lib/mcast-snooping.h | 5 +
lib/packets.h| 26 ++
ofproto/ofproto-dpif-xlate.c | 19
4 files changed, 98 insertions(+), 4 deletions(-)
diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
index c3ffd6b..2067a32 100644
--- a/lib/mcast-snooping.c
+++ b/lib/mcast-snooping.c
@@ -69,6 +69,7 @@ mcast_snooping_is_membership(ovs_be16 igmp_type)
switch (ntohs(igmp_type)) {
case IGMP_HOST_MEMBERSHIP_REPORT:
case IGMPV2_HOST_MEMBERSHIP_REPORT:
+case IGMPV3_HOST_MEMBERSHIP_REPORT:
case IGMP_HOST_LEAVE_MESSAGE:
return true;
}
@@ -416,6 +417,57 @@ mcast_snooping_add_group(struct mcast_snooping *ms,
ovs_be32 ip4,
return learned;
}
+int
+mcast_snooping_add_report(struct mcast_snooping *ms,
+ const struct dp_packet *p,
+ uint16_t vlan, void *port)
+{
+ovs_be32 ip4;
+size_t offset;
+const struct igmpv3_header *igmpv3;
+const struct igmpv3_record *record;
+int count = 0;
+int ngrp;
+
+offset = (char *) dp_packet_l4(p) - (char *) dp_packet_data(p);
+igmpv3 = dp_packet_at(p, offset, IGMPV3_HEADER_LEN);
+if (!igmpv3) {
+return 0;
+}
+ngrp = ntohs(igmpv3->ngrp);
+offset += IGMPV3_HEADER_LEN;
+while (ngrp--) {
+bool ret;
+record = dp_packet_at(p, offset, sizeof(struct igmpv3_record));
+if (!record) {
+break;
+}
+/* Only consider known record types. */
+if (record->type < IGMPV3_MODE_IS_INCLUDE
+|| record->type > IGMPV3_BLOCK_OLD_SOURCES) {
+continue;
+}
+ip4 = get_16aligned_be32(&record->maddr);
+/*
+ * If record is INCLUDE MODE and there are no sources, it's equivalent
+ * to a LEAVE.
+ * */
+if (ntohs(record->nsrcs) == 0
+&& (record->type == IGMPV3_MODE_IS_INCLUDE
+|| record->type == IGMPV3_CHANGE_TO_INCLUDE_MODE)) {
+ret = mcast_snooping_leave_group(ms, ip4, vlan, port);
+} else {
+ret = mcast_snooping_add_group(ms, ip4, vlan, port);
+}
+if (ret) {
+count++;
+}
+offset += sizeof(*record)
+ + ntohs(record->nsrcs) * sizeof(ovs_be32) + record->aux_len;
+}
+return count;
+}
+
bool
mcast_snooping_leave_group(struct mcast_snooping *ms, ovs_be32 ip4,
uint16_t vlan, void *port)
diff --git a/lib/mcast-snooping.h b/lib/mcast-snooping.h
index 979b2aa..f4bc8fb 100644
--- a/lib/mcast-snooping.h
+++ b/lib/mcast-snooping.h
@@ -20,6 +20,7 @@
#define MCAST_SNOOPING_H 1
#include
+#include "dp-packet.h"
#include "hmap.h"
#include "list.h"
#include "ovs-atomic.h"
@@ -181,6 +182,10 @@ mcast_snooping_lookup(const struct mcast_snooping *ms,
ovs_be32 dip,
bool mcast_snooping_add_group(struct mcast_snooping *ms, ovs_be32 ip4,
uint16_t vlan, void *port)
OVS_REQ_WRLOCK(ms->rwlock);
+int mcast_snooping_add_report(struct mcast_snooping *ms,
+ const struct dp_packet *p,
+ uint16_t vlan, void *port)
+OVS_REQ_WRLOCK(ms->rwlock);
bool mcast_snooping_leave_group(struct mcast_snooping *ms, ovs_be32 ip4,
uint16_t vlan, void *port)
OVS_REQ_WRLOCK(ms->rwlock);
diff --git a/lib/packets.h b/lib/packets.h
index e22267e..63ad2ff 100644
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -540,12 +540,38 @@ struct igmp_header {
};
BUILD_ASSERT_DECL(IGMP_HEADER_LEN == sizeof(struct igmp_header));
+#define IGMPV3_HEADER_LEN 8
+struct igmpv3_header {
+uint8_t type;
+uint8_t rsvr1;
+ovs_be16 csum;
+ovs_be16 rsvr2;
+ovs_be16 ngrp;
+};
+BUILD_ASSERT_DECL(IGMPV3_HEADER_LEN == sizeof(struct igmpv3_header));
+
+#define IGMPV3_RECORD_LEN 8
+struct igmpv3_record {
+uint8_t type;
+uint8_t aux_len;
+ovs_be16 nsrcs;
+ovs_16aligned_be32 maddr;
+};
+BUILD_ASSERT_DECL(IGMPV3_RECORD_LEN == sizeof(struct igmpv3_record));
+
#define IGMP_HOST_MEMBERSHIP_QUERY0x11 /* From RFC1112 */
#define IGMP_HOST_MEMBERSHIP_REPORT 0x12 /* Ditto */
#define IGMPV2_HOST_MEMBERSHIP_REPORT 0x16 /* V2 version of 0x12 */
#define IGMP_HOST_LEAVE_MESSAGE 0x17
#define IGMPV3_HOST_MEMBERSHIP_REPORT 0x22 /* V3 version of 0x12 */
+#define IGMPV3_MODE_IS_I
Re: [ovs-dev] [PATCH] dpif-netdev: Check for PKT_RX_RSS_HASH flag.
On Tue, Jun 16, 2015 at 07:39:00PM +0100, Daniele Di Proietto wrote:
> DPDK mbufs contain a valid RSS hash only if PKT_RX_RSS_HASH is
> set in 'ol_flags'. Otherwise the hash is garbage and doesn't
> relate to the packet.
>
> This fixes an issue with vhost, which, being a virtual NIC, doesn't
> compute the hash.
>
> Unfortunately the ixgbe vPMD doesn't set the PKT_RX_RSS_HASH, forcing
> OVS to compute an hash is software. This has a significant impact on
> performance (-30% throughput in a single flow setup) which can be
> mitigated in the CPU supports crc32c instructions.
>
> Reported-by: Dongjun
> Suggested-by: Flavio Leitner
> Signed-off-by: Daniele Di Proietto
> ---
> lib/dp-packet.h | 11 +++
> lib/dpif-netdev.c | 2 +-
> 2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> index e4c2593..6840750 100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -529,11 +529,22 @@ dp_packet_set_rss_hash(struct dp_packet *p, uint32_t
> hash)
> {
> #ifdef DPDK_NETDEV
> p->mbuf.hash.rss = hash;
> +p->mbuf.ol_flags |= PKT_RX_RSS_HASH;
> #else
> p->rss_hash = hash;
> #endif
> }
>
> +static inline bool
> +dp_packet_rss_valid(struct dp_packet *p)
> +{
> +#ifdef DPDK_NETDEV
> +return p->mbuf.ol_flags & PKT_RX_RSS_HASH;
> +#else
> +return true;
> +#endif
> +}
> +
> #ifdef __cplusplus
> }
> #endif
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index f13169c..c4a4b3a 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -3036,7 +3036,7 @@ dpif_netdev_packet_get_rss_hash(struct dp_packet
> *packet,
> {
> uint32_t hash, recirc_depth;
>
> -hash = dp_packet_get_rss_hash(packet);
> +hash = dp_packet_rss_valid(packet) ? dp_packet_get_rss_hash(packet) : 0;
> if (OVS_UNLIKELY(!hash)) {
> hash = miniflow_hash_5tuple(mf, 0);
> dp_packet_set_rss_hash(packet, hash);
> --
> 2.1.4
Callers of dp_packet_get_rss_hash() have no idea about the ol_flags,
so this could lead to other problems in the future. But the
alternative would be to hash at dp_packet_get_rss_hash() which
doesn't have parsed data in miniflow/flow.
So, could you please add a comment on top of dp_packet_get_rss_hash()
about dp_packet_rss_valid()?
Otherwise the patch looks good to me.
Thanks!
fbl
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] Make IGMP packets always take slow path
On Tue, Jun 16, 2015 at 05:16:19PM -0300, Thadeu Lima de Souza Cascardo wrote:
> IGMP packets need to take the slow path. Otherwise, packets that match
> the same flow will not be processed by OVS. That might prevent OVS from
> updating the expire time for entries already in the mdb, but also to
> lose packets with different addresses in the payload.
>
> Signed-off-by: Thadeu Lima de Souza Cascardo
> ---
> ofproto/ofproto-dpif-xlate.c | 14 ++
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index f5dc272..0b113a2 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2269,12 +2269,18 @@ xlate_normal(struct xlate_ctx *ctx)
> struct mcast_group *grp;
>
> if (flow->nw_proto == IPPROTO_IGMP) {
> -if (ctx->xin->may_learn) {
> -if (mcast_snooping_is_membership(flow->tp_src) ||
> -mcast_snooping_is_query(flow->tp_src)) {
> +if (mcast_snooping_is_membership(flow->tp_src) ||
> +mcast_snooping_is_query(flow->tp_src)) {
> +if (ctx->xin->may_learn) {
> update_mcast_snooping_table(ctx->xbridge, flow, vlan,
> in_xbundle);
> -}
> +}
> +/*
> + * IGMP packets need to take the slow path, in order to be
> + * processed for mdb updates. That will prevent expires
> +* firing off even after hosts have sent reports.
> +*/
There is a indentation problem above
fbl
> +ctx->xout->slow |= SLOW_ACTION;
> }
>
> if (mcast_snooping_is_membership(flow->tp_src)) {
> --
> 2.4.2
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] IGMPv3 support
On Tue, Jun 16, 2015 at 05:16:20PM -0300, Thadeu Lima de Souza Cascardo wrote:
> Support IGMPv3 messages with multiple records. Make sure all IGMPv3
> messages go through slow path, since they may carry multiple multicast
> addresses, unlike IGMPv2.
>
> Tests done:
>
> * multiple addresses in IGMPv3 report are inserted in mdb;
> * address is removed from IGMPv3 if record is INCLUDE_MODE;
> * reports sent on a burst with same flow all go to userspace;
> * IGMPv3 reports go to mrouters, i.e., ports that have issued a query.
>
> Signed-off-by: Thadeu Lima de Souza Cascardo
> ---
> lib/mcast-snooping.c | 52
>
> lib/mcast-snooping.h | 5 +
> lib/packets.h| 26 ++
> ofproto/ofproto-dpif-xlate.c | 19
> 4 files changed, 98 insertions(+), 4 deletions(-)
>
> diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
> index c3ffd6b..2067a32 100644
> --- a/lib/mcast-snooping.c
> +++ b/lib/mcast-snooping.c
> @@ -69,6 +69,7 @@ mcast_snooping_is_membership(ovs_be16 igmp_type)
> switch (ntohs(igmp_type)) {
> case IGMP_HOST_MEMBERSHIP_REPORT:
> case IGMPV2_HOST_MEMBERSHIP_REPORT:
> +case IGMPV3_HOST_MEMBERSHIP_REPORT:
> case IGMP_HOST_LEAVE_MESSAGE:
> return true;
> }
> @@ -416,6 +417,57 @@ mcast_snooping_add_group(struct mcast_snooping *ms,
> ovs_be32 ip4,
> return learned;
> }
>
> +int
> +mcast_snooping_add_report(struct mcast_snooping *ms,
> + const struct dp_packet *p,
> + uint16_t vlan, void *port)
> +{
> +ovs_be32 ip4;
> +size_t offset;
> +const struct igmpv3_header *igmpv3;
> +const struct igmpv3_record *record;
> +int count = 0;
> +int ngrp;
> +
> +offset = (char *) dp_packet_l4(p) - (char *) dp_packet_data(p);
> +igmpv3 = dp_packet_at(p, offset, IGMPV3_HEADER_LEN);
> +if (!igmpv3) {
> +return 0;
> +}
> +ngrp = ntohs(igmpv3->ngrp);
> +offset += IGMPV3_HEADER_LEN;
> +while (ngrp--) {
> +bool ret;
> +record = dp_packet_at(p, offset, sizeof(struct igmpv3_record));
> +if (!record) {
> +break;
> +}
> +/* Only consider known record types. */
> +if (record->type < IGMPV3_MODE_IS_INCLUDE
> +|| record->type > IGMPV3_BLOCK_OLD_SOURCES) {
> +continue;
> +}
> +ip4 = get_16aligned_be32(&record->maddr);
> +/*
> + * If record is INCLUDE MODE and there are no sources, it's
> equivalent
> + * to a LEAVE.
> + * */
> +if (ntohs(record->nsrcs) == 0
> +&& (record->type == IGMPV3_MODE_IS_INCLUDE
> +|| record->type == IGMPV3_CHANGE_TO_INCLUDE_MODE)) {
> +ret = mcast_snooping_leave_group(ms, ip4, vlan, port);
> +} else {
> +ret = mcast_snooping_add_group(ms, ip4, vlan, port);
> +}
> +if (ret) {
> +count++;
> +}
> +offset += sizeof(*record)
> + + ntohs(record->nsrcs) * sizeof(ovs_be32) +
> record->aux_len;
> +}
> +return count;
> +}
> +
> bool
> mcast_snooping_leave_group(struct mcast_snooping *ms, ovs_be32 ip4,
> uint16_t vlan, void *port)
> diff --git a/lib/mcast-snooping.h b/lib/mcast-snooping.h
> index 979b2aa..f4bc8fb 100644
> --- a/lib/mcast-snooping.h
> +++ b/lib/mcast-snooping.h
> @@ -20,6 +20,7 @@
> #define MCAST_SNOOPING_H 1
>
> #include
> +#include "dp-packet.h"
> #include "hmap.h"
> #include "list.h"
> #include "ovs-atomic.h"
> @@ -181,6 +182,10 @@ mcast_snooping_lookup(const struct mcast_snooping *ms,
> ovs_be32 dip,
> bool mcast_snooping_add_group(struct mcast_snooping *ms, ovs_be32 ip4,
>uint16_t vlan, void *port)
> OVS_REQ_WRLOCK(ms->rwlock);
> +int mcast_snooping_add_report(struct mcast_snooping *ms,
> + const struct dp_packet *p,
> + uint16_t vlan, void *port)
> +OVS_REQ_WRLOCK(ms->rwlock);
> bool mcast_snooping_leave_group(struct mcast_snooping *ms, ovs_be32 ip4,
> uint16_t vlan, void *port)
> OVS_REQ_WRLOCK(ms->rwlock);
> diff --git a/lib/packets.h b/lib/packets.h
> index e22267e..63ad2ff 100644
> --- a/lib/packets.h
> +++ b/lib/packets.h
> @@ -540,12 +540,38 @@ struct igmp_header {
> };
> BUILD_ASSERT_DECL(IGMP_HEADER_LEN == sizeof(struct igmp_header));
>
> +#define IGMPV3_HEADER_LEN 8
> +struct igmpv3_header {
> +uint8_t type;
> +uint8_t rsvr1;
> +ovs_be16 csum;
> +ovs_be16 rsvr2;
> +ovs_be16 ngrp;
> +};
> +BUILD_ASSERT_DECL(IGMPV3_HEADER_LEN == sizeof(struct igmpv3_header));
> +
> +#define IGMPV3_RECORD_LEN 8
> +struct igmpv3_record {
> +uint8_t type;
> +uint8_t aux_len;
> +ovs_be16 nsrcs;
> +ovs_16aligned_be32 maddr;
> +};
> +BUILD_ASSERT_DECL(IGMPV3_RECORD_
Re: [ovs-dev] Is this an issue for DPDK vhost rss?
On Tue, Jun 16, 2015 at 06:48:13PM +, Daniele Di Proietto wrote: > On 16/06/2015 18:27, "Flavio Leitner" wrote: > >Could you point me to the vector and non-vector rx routines? > >I feel like I am missing something. > > ixgbe_recv_pkts_vec() in drivers/net/ixgbe/ixgbe_rxtx.c is the vector rx. > There are multiple version of the non-vector rx routine, I believe we used > ixgbe_recv_pkts() in drivers/net/ixgbe/ixgbe_rxtx.c Thanks for the pointers! fbl ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v4 2/2] IGMPv3 support
Support IGMPv3 messages with multiple records. Make sure all IGMPv3
messages go through slow path, since they may carry multiple multicast
addresses, unlike IGMPv2.
Tests done:
* multiple addresses in IGMPv3 report are inserted in mdb;
* address is removed from IGMPv3 if record is INCLUDE_MODE;
* reports sent on a burst with same flow all go to userspace;
* IGMPv3 reports go to mrouters, i.e., ports that have issued a query.
Signed-off-by: Thadeu Lima de Souza Cascardo
---
lib/mcast-snooping.c | 52
lib/mcast-snooping.h | 5 +
lib/packets.h| 26 ++
ofproto/ofproto-dpif-xlate.c | 19
4 files changed, 98 insertions(+), 4 deletions(-)
diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
index c3ffd6b..7b927aa 100644
--- a/lib/mcast-snooping.c
+++ b/lib/mcast-snooping.c
@@ -69,6 +69,7 @@ mcast_snooping_is_membership(ovs_be16 igmp_type)
switch (ntohs(igmp_type)) {
case IGMP_HOST_MEMBERSHIP_REPORT:
case IGMPV2_HOST_MEMBERSHIP_REPORT:
+case IGMPV3_HOST_MEMBERSHIP_REPORT:
case IGMP_HOST_LEAVE_MESSAGE:
return true;
}
@@ -416,6 +417,57 @@ mcast_snooping_add_group(struct mcast_snooping *ms,
ovs_be32 ip4,
return learned;
}
+int
+mcast_snooping_add_report(struct mcast_snooping *ms,
+ const struct dp_packet *p,
+ uint16_t vlan, void *port)
+{
+ovs_be32 ip4;
+size_t offset;
+const struct igmpv3_header *igmpv3;
+const struct igmpv3_record *record;
+int count = 0;
+int ngrp;
+
+offset = (char *) dp_packet_l4(p) - (char *) dp_packet_data(p);
+igmpv3 = dp_packet_at(p, offset, IGMPV3_HEADER_LEN);
+if (!igmpv3) {
+return 0;
+}
+ngrp = ntohs(igmpv3->ngrp);
+offset += IGMPV3_HEADER_LEN;
+while (ngrp--) {
+bool ret;
+record = dp_packet_at(p, offset, sizeof(struct igmpv3_record));
+if (!record) {
+break;
+}
+/* Only consider known record types. */
+if (record->type < IGMPV3_MODE_IS_INCLUDE
+|| record->type > IGMPV3_BLOCK_OLD_SOURCES) {
+continue;
+}
+ip4 = get_16aligned_be32(&record->maddr);
+/*
+ * If record is INCLUDE MODE and there are no sources, it's equivalent
+ * to a LEAVE.
+ */
+if (ntohs(record->nsrcs) == 0
+&& (record->type == IGMPV3_MODE_IS_INCLUDE
+|| record->type == IGMPV3_CHANGE_TO_INCLUDE_MODE)) {
+ret = mcast_snooping_leave_group(ms, ip4, vlan, port);
+} else {
+ret = mcast_snooping_add_group(ms, ip4, vlan, port);
+}
+if (ret) {
+count++;
+}
+offset += sizeof(*record)
+ + ntohs(record->nsrcs) * sizeof(ovs_be32) + record->aux_len;
+}
+return count;
+}
+
bool
mcast_snooping_leave_group(struct mcast_snooping *ms, ovs_be32 ip4,
uint16_t vlan, void *port)
diff --git a/lib/mcast-snooping.h b/lib/mcast-snooping.h
index 979b2aa..f4bc8fb 100644
--- a/lib/mcast-snooping.h
+++ b/lib/mcast-snooping.h
@@ -20,6 +20,7 @@
#define MCAST_SNOOPING_H 1
#include
+#include "dp-packet.h"
#include "hmap.h"
#include "list.h"
#include "ovs-atomic.h"
@@ -181,6 +182,10 @@ mcast_snooping_lookup(const struct mcast_snooping *ms,
ovs_be32 dip,
bool mcast_snooping_add_group(struct mcast_snooping *ms, ovs_be32 ip4,
uint16_t vlan, void *port)
OVS_REQ_WRLOCK(ms->rwlock);
+int mcast_snooping_add_report(struct mcast_snooping *ms,
+ const struct dp_packet *p,
+ uint16_t vlan, void *port)
+OVS_REQ_WRLOCK(ms->rwlock);
bool mcast_snooping_leave_group(struct mcast_snooping *ms, ovs_be32 ip4,
uint16_t vlan, void *port)
OVS_REQ_WRLOCK(ms->rwlock);
diff --git a/lib/packets.h b/lib/packets.h
index e22267e..63ad2ff 100644
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -540,12 +540,38 @@ struct igmp_header {
};
BUILD_ASSERT_DECL(IGMP_HEADER_LEN == sizeof(struct igmp_header));
+#define IGMPV3_HEADER_LEN 8
+struct igmpv3_header {
+uint8_t type;
+uint8_t rsvr1;
+ovs_be16 csum;
+ovs_be16 rsvr2;
+ovs_be16 ngrp;
+};
+BUILD_ASSERT_DECL(IGMPV3_HEADER_LEN == sizeof(struct igmpv3_header));
+
+#define IGMPV3_RECORD_LEN 8
+struct igmpv3_record {
+uint8_t type;
+uint8_t aux_len;
+ovs_be16 nsrcs;
+ovs_16aligned_be32 maddr;
+};
+BUILD_ASSERT_DECL(IGMPV3_RECORD_LEN == sizeof(struct igmpv3_record));
+
#define IGMP_HOST_MEMBERSHIP_QUERY0x11 /* From RFC1112 */
#define IGMP_HOST_MEMBERSHIP_REPORT 0x12 /* Ditto */
#define IGMPV2_HOST_MEMBERSHIP_REPORT 0x16 /* V2 version of 0x12 */
#define IGMP_HOST_LEAVE_MESSAGE 0x17
#define IGMPV3_HOST_MEMBERSHIP_REPORT 0x22 /* V3 version of 0x12 */
+#define IGMPV3_MODE_IS_INC
[ovs-dev] [PATCH v4 1/2] Make IGMP packets always take slow path
IGMP packets need to take the slow path. Otherwise, packets that match
the same flow will not be processed by OVS. That might prevent OVS from
updating the expire time for entries already in the mdb, but also to
lose packets with different addresses in the payload.
Signed-off-by: Thadeu Lima de Souza Cascardo
---
ofproto/ofproto-dpif-xlate.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index f5dc272..6fcf7b8 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2269,12 +2269,18 @@ xlate_normal(struct xlate_ctx *ctx)
struct mcast_group *grp;
if (flow->nw_proto == IPPROTO_IGMP) {
-if (ctx->xin->may_learn) {
-if (mcast_snooping_is_membership(flow->tp_src) ||
-mcast_snooping_is_query(flow->tp_src)) {
+if (mcast_snooping_is_membership(flow->tp_src) ||
+mcast_snooping_is_query(flow->tp_src)) {
+if (ctx->xin->may_learn) {
update_mcast_snooping_table(ctx->xbridge, flow, vlan,
in_xbundle);
-}
+}
+/*
+ * IGMP packets need to take the slow path, in order to be
+ * processed for mdb updates. That will prevent expires
+ * firing off even after hosts have sent reports.
+*/
+ctx->xout->slow |= SLOW_ACTION;
}
if (mcast_snooping_is_membership(flow->tp_src)) {
--
2.4.2
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v4 1/2] Make IGMP packets always take slow path
On Tue, Jun 16, 2015 at 06:01:09PM -0300, Thadeu Lima de Souza Cascardo wrote:
> IGMP packets need to take the slow path. Otherwise, packets that match
> the same flow will not be processed by OVS. That might prevent OVS from
> updating the expire time for entries already in the mdb, but also to
> lose packets with different addresses in the payload.
>
> Signed-off-by: Thadeu Lima de Souza Cascardo
> ---
> ofproto/ofproto-dpif-xlate.c | 14 ++
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index f5dc272..6fcf7b8 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2269,12 +2269,18 @@ xlate_normal(struct xlate_ctx *ctx)
> struct mcast_group *grp;
>
> if (flow->nw_proto == IPPROTO_IGMP) {
> -if (ctx->xin->may_learn) {
> -if (mcast_snooping_is_membership(flow->tp_src) ||
> -mcast_snooping_is_query(flow->tp_src)) {
> +if (mcast_snooping_is_membership(flow->tp_src) ||
> +mcast_snooping_is_query(flow->tp_src)) {
> +if (ctx->xin->may_learn) {
> update_mcast_snooping_table(ctx->xbridge, flow, vlan,
> in_xbundle);
> -}
> +}
> +/*
> + * IGMP packets need to take the slow path, in order to be
> + * processed for mdb updates. That will prevent expires
> + * firing off even after hosts have sent reports.
> +*/
looks like the above isn't correct aligned.
I forgot to tell before about updating the NEWS file:
- Support for multicast snooping (IGMPv1 and IGMPv2
and to update vswitchd/vswitch.xml:
Multicast snooping (RFC 4541) monitors the Internet Group Management
Protocol (IGMP) traffic between hosts and multicast routers. The
switch uses what IGMP snooping learns to forward multicast traffic
only to interfaces that are connected to interested receivers.
Currently it supports IGMPv1 and IGMPv2 protocols.
fbl
> +ctx->xout->slow |= SLOW_ACTION;
> }
>
> if (mcast_snooping_is_membership(flow->tp_src)) {
> --
> 2.4.2
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 1/1] auto-attach: Cleanup i-sid/vlan mappings associated with lldp-enabled port.
From: Dennis Flynn
This commit fixes a bug where the i-sid/vlan mapping structures associated with
an lldp-enabled port were not being freed during general port cleanup.
Signed-off-by: Dennis Flynn
diff --git a/lib/lldp/lldpd-structs.c b/lib/lldp/lldpd-structs.c
index b78c2e1..71c4f5e 100644
--- a/lib/lldp/lldpd-structs.c
+++ b/lib/lldp/lldpd-structs.c
@@ -93,17 +93,41 @@ lldpd_remote_cleanup(struct lldpd_hardware *hw,
}
}
+/* Cleanup the auto-attach mappings attached to port.
+ */
+static void
+lldpd_aa_maps_cleanup(struct lldpd_port *port)
+{
+struct lldpd_aa_isid_vlan_maps_tlv *isid_vlan_map = NULL;
+struct lldpd_aa_isid_vlan_maps_tlv *isid_vlan_map_next = NULL;
+
+if (!list_is_empty(&port->p_isid_vlan_maps)) {
+
+LIST_FOR_EACH_SAFE (isid_vlan_map, isid_vlan_map_next, m_entries,
+&port->p_isid_vlan_maps) {
+
+list_remove(&isid_vlan_map->m_entries);
+free(isid_vlan_map);
+}
+
+list_init(&port->p_isid_vlan_maps);
+}
+}
+
/* If `all' is true, clear all information, including information that
are not refreshed periodically. Port should be freed manually. */
void
lldpd_port_cleanup(struct lldpd_port *port, bool all)
{
/* We set these to NULL so we don't free wrong memory */
-
free(port->p_id);
port->p_id = NULL;
free(port->p_descr);
port->p_descr = NULL;
+
+/* Cleanup auto-attach mappings */
+lldpd_aa_maps_cleanup(port);
+
if (all) {
free(port->p_lastframe);
/* Chassis may not have been attributed, yet.*/
--
1.8.3.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v4 1/2] Make IGMP packets always take slow path
On Tue, Jun 16, 2015 at 06:25:47PM -0300, Flavio Leitner wrote:
> On Tue, Jun 16, 2015 at 06:01:09PM -0300, Thadeu Lima de Souza Cascardo wrote:
> > IGMP packets need to take the slow path. Otherwise, packets that match
> > the same flow will not be processed by OVS. That might prevent OVS from
> > updating the expire time for entries already in the mdb, but also to
> > lose packets with different addresses in the payload.
> >
> > Signed-off-by: Thadeu Lima de Souza Cascardo
> > ---
> > ofproto/ofproto-dpif-xlate.c | 14 ++
> > 1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index f5dc272..6fcf7b8 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -2269,12 +2269,18 @@ xlate_normal(struct xlate_ctx *ctx)
> > struct mcast_group *grp;
> >
> > if (flow->nw_proto == IPPROTO_IGMP) {
> > -if (ctx->xin->may_learn) {
> > -if (mcast_snooping_is_membership(flow->tp_src) ||
> > -mcast_snooping_is_query(flow->tp_src)) {
> > +if (mcast_snooping_is_membership(flow->tp_src) ||
> > +mcast_snooping_is_query(flow->tp_src)) {
> > +if (ctx->xin->may_learn) {
> > update_mcast_snooping_table(ctx->xbridge, flow, vlan,
> > in_xbundle);
> > -}
> > +}
> > +/*
> > + * IGMP packets need to take the slow path, in order to be
> > + * processed for mdb updates. That will prevent expires
> > + * firing off even after hosts have sent reports.
> > +*/
>
> looks like the above isn't correct aligned.
>
> I forgot to tell before about updating the NEWS file:
>
> - Support for multicast snooping (IGMPv1 and IGMPv2
>
> and to update vswitchd/vswitch.xml:
>
> Multicast snooping (RFC 4541) monitors the Internet Group Management
> Protocol (IGMP) traffic between hosts and multicast routers. The
> switch uses what IGMP snooping learns to forward multicast traffic
> only to interfaces that are connected to interested receivers.
> Currently it supports IGMPv1 and IGMPv2 protocols.
>
> fbl
>
Thanks a lot for the review, Flavio.
Excuse my lack of attention to the details. I miss something like
checkpatch on Linux, that would take care of some of the coding style
problems one may find in a patch. Is there any recommendations on that
for OVS?
I have a v5 prepared, but will wait for any other comments before I send
it.
Regards.
Cascardo.
>
> > +ctx->xout->slow |= SLOW_ACTION;
> > }
> >
> > if (mcast_snooping_is_membership(flow->tp_src)) {
> > --
> > 2.4.2
> >
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v4 1/2] Make IGMP packets always take slow path
On Tue, Jun 16, 2015 at 07:09:25PM -0300, Thadeu Lima de Souza Cascardo wrote:
> On Tue, Jun 16, 2015 at 06:25:47PM -0300, Flavio Leitner wrote:
> > On Tue, Jun 16, 2015 at 06:01:09PM -0300, Thadeu Lima de Souza Cascardo
> > wrote:
> > > IGMP packets need to take the slow path. Otherwise, packets that match
> > > the same flow will not be processed by OVS. That might prevent OVS from
> > > updating the expire time for entries already in the mdb, but also to
> > > lose packets with different addresses in the payload.
> > >
> > > Signed-off-by: Thadeu Lima de Souza Cascardo
> > > ---
> > > ofproto/ofproto-dpif-xlate.c | 14 ++
> > > 1 file changed, 10 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > > index f5dc272..6fcf7b8 100644
> > > --- a/ofproto/ofproto-dpif-xlate.c
> > > +++ b/ofproto/ofproto-dpif-xlate.c
> > > @@ -2269,12 +2269,18 @@ xlate_normal(struct xlate_ctx *ctx)
> > > struct mcast_group *grp;
> > >
> > > if (flow->nw_proto == IPPROTO_IGMP) {
> > > -if (ctx->xin->may_learn) {
> > > -if (mcast_snooping_is_membership(flow->tp_src) ||
> > > -mcast_snooping_is_query(flow->tp_src)) {
> > > +if (mcast_snooping_is_membership(flow->tp_src) ||
> > > +mcast_snooping_is_query(flow->tp_src)) {
> > > +if (ctx->xin->may_learn) {
> > > update_mcast_snooping_table(ctx->xbridge, flow, vlan,
> > > in_xbundle);
> > > -}
> > > +}
> > > +/*
> > > + * IGMP packets need to take the slow path, in order to
> > > be
> > > + * processed for mdb updates. That will prevent expires
> > > + * firing off even after hosts have sent reports.
> > > +*/
> >
> > looks like the above isn't correct aligned.
> >
> > I forgot to tell before about updating the NEWS file:
> >
> > - Support for multicast snooping (IGMPv1 and IGMPv2
> >
> > and to update vswitchd/vswitch.xml:
> >
> > Multicast snooping (RFC 4541) monitors the Internet Group Management
> > Protocol (IGMP) traffic between hosts and multicast routers. The
> > switch uses what IGMP snooping learns to forward multicast traffic
> > only to interfaces that are connected to interested receivers.
> > Currently it supports IGMPv1 and IGMPv2 protocols.
> >
> > fbl
> >
>
> Thanks a lot for the review, Flavio.
>
> Excuse my lack of attention to the details. I miss something like
> checkpatch on Linux, that would take care of some of the coding style
> problems one may find in a patch. Is there any recommendations on that
> for OVS?
A checkpatch script for OVS would be very welcome.
> I have a v5 prepared, but will wait for any other comments before I send
> it.
Sounds good to me.
Perhaps Ben has something else to add.
Thanks Thadeu,
fbl
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] Do not flush tx queue which is shared among CPUs since it is always flushed
On 2015/6/17 1:44, Daniele Di Proietto wrote:
On 16/06/2015 07:40, "Pravin Shelar" wrote:
On Mon, Jun 8, 2015 at 7:42 PM, Pravin Shelar wrote:
On Mon, Jun 8, 2015 at 6:13 PM, Wei li wrote:
When tx queue is shared among CPUS,the pkts always be flush in
'netdev_dpdk_eth_send'
So it is unnecessarily for flushing in netdev_dpdk_rxq_recv
Otherwise tx will be accessed without locking
Signed-off-by: Wei li
---
v1->v2: fix typo
lib/netdev-dpdk.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 63243d8..1e0a483 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -892,8 +892,11 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq_,
struct dp_packet **packets,
int nb_rx;
/* There is only one tx queue for this core. Do not flush other
- * queueus. */
-if (rxq_->queue_id == rte_lcore_id()) {
+ * queues.
+ * Do not flush tx queue which is shared among CPUs
+ * since it is always flushed */
+if (rxq_->queue_id == rte_lcore_id() &&
+ OVS_LIKELY(!dev->txq_needs_locking)) {
dpdk_queue_flush(dev, rxq_->queue_id);
}
Patch looks good, But Daniele has plan to get rid of queue flushing
logic. do you see problem with doing this?
Daniele,
I am not sure if we should fix this queue flushing logic if we are
going to remove it. So I would like to discuss it first.
You mentioned that the removing flushing logic results in performance
drop. Can you explain it? How much is performance drop and which is
the case?
Hi Pravin,
sorry for the late reply. I suspected that removing the queues in
netdev-dpdk
was going to have a performance impact in the following scenario: a batch
of
packets from different megaflows with the same action (output on port 1).
Here's what happens:
1) With the queues in netdev-dpdk. dpif-netdev calls netdev_send() for
each
packet. netdev_dpdk_send() just copies the pointer in the queue. Before
receiving the next batch dpdk_queue_flush() call rte_eth_tx_burst() with
a whole batch of packets
2) Without the queues in netdev-dpdk. dpif-netdev calls netdev_send() for
each
packet. netdev_dpdk_send() calls rte_eth_tx_burst() for each packet.
Scenario 2 could be slower because rte_eth_tx_burst() is called for each
packet
(instead of each batch). After some testing this turns out not to be the
case.
It appears that the bottleneck is not rte_eth_tx_burst(), and copying the
pointers
in the temporary queue makes the code slower.
Hi Daniele,
I get something else impacting the performance too. Here it is:
In netdev_dpdk_rxq_recv(), the flush condition is "txq->count != 0", and
in dpdk_queue_pkts(), the flush condition is "txq->count ==
MAX_TX_QUEUE_LEN" or "(rte_get_timer_cycles() - txq->tsc) >= DRAIN_TSC".
They don't match. Our thoughts is to get a batch flush, but
netdev_dpdk_rxq_recv() may do it earlier sometime.
I did some local change, stealing the cycle interval from
dpdk_queue_pkts(). I got performance improvement, about at least 10%
from guest VNIC to dpdk phy NIC to outside.
I didn't compare the performance to "Scenario 2" for "Scenario 2"
modification is not that easy. :(
$ git diff
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 3af1ee7..98d33c3 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -929,10 +929,13 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq_,
struct dp_packet **packets,
/* There is only one tx queue for this core. Do not flush other
* queueus. */
-if (rxq_->queue_id == rte_lcore_id()) {
-dpdk_queue_flush(dev, rxq_->queue_id);
-}
+if (rxq_->queue_id == rte_lcore_id() &&
OVS_LIKELY(!dev->txq_needs_locking)) {
+struct dpdk_tx_queue *txq = &dev->tx_q[rxq_->queue_id];
+if (txq->count != 0 && (rte_get_timer_cycles() - txq->tsc) >=
DRAIN_TSC) {
+dpdk_queue_flush__(dev, rxq_->queue_id); // instead of
dpdk_queue_flush
+}
+}
nb_rx = rte_eth_rx_burst(rx->port_id, rxq_->queue_id,
(struct rte_mbuf **) packets,
NETDEV_MAX_BURST);
What do you think? Should we just remove the queues?
CC'ing Mark since he expressed interest in the issue
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] fix wrong cmd for create dpdkvhostuser in INSTALL.DPDK.md
On Tuesday 16 June 2015 11:07 PM, Flavio Leitner wrote: On Tue, Jun 16, 2015 at 11:23:07AM +0530, gowrishankar wrote: Also updated an additional correction as in : http://openvswitch.org/pipermail/dev/2015-June/056379.html Could you include that as well. Yeap. One patch with all the s/ovs-vsctl/ovs-vsctl/ and another for the scheme initialization? Thanks, fbl Latter change can be dropped as I did not 'make install' to refer schema in installed path. Thanks Flavio. Regards, Gowri Shankar ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH/RFC] connmgr: clear ofproto's pointer to connmgr when the latter is destroyed
As per the testcase included in this patch it has been observed
that ovs-vswtichd may segfault when deleting a bridge.
Analysing the output of valgrind and gdb it appears that
this is caused by the connmgr of a ofproto being accessed
after the latter has been freed.
It appears that this is occurring in different threads and
is the result of the following postponement arrangement in ofproto_destroy();
/* We should not postpone this because it involves deleting a listening
* socket which we may want to reopen soon. 'connmgr' should not be used
* by other threads */
connmgr_destroy(p->connmgr);
/* Destroying rules is deferred, must have 'ofproto' around for them. */
ovsrcu_postpone(ofproto_destroy__, p);
Report from valgrind resulting from make check-valgrind:
==10653== Invalid read of size 8
==10653==at 0x458998: ofmonitor_flush (connmgr.c:2254)
==10653==by 0x423E17: handle_flow_mod__ (ofproto.c:4942)
==10653==by 0x42651E: delete_group__ (ofproto.c:6171)
==10653==by 0x426600: delete_group (ofproto.c:6194)
==10653==by 0x41BD37: ofproto_destroy__ (ofproto.c:1485)
==10653==by 0x4EF80E: ovsrcu_call_postponed (ovs-rcu.c:256)
==10653==by 0x4EF8B4: ovsrcu_postpone_thread (ovs-rcu.c:271)
==10653==by 0x4F1656: ovsthread_wrapper (ovs-thread.c:337)
==10653==by 0x569F0A3: start_thread (pthread_create.c:309)
==10653==by 0x5EA304C: clone (clone.S:111)
==10653== Address 0x645a910 is 64 bytes inside a block of size 184 free'd
==10653==at 0x4C29E90: free (vg_replace_malloc.c:473)
==10653==by 0x45494F: connmgr_destroy (connmgr.c:299)
==10653==by 0x41C0E6: ofproto_destroy (ofproto.c:1553)
==10653==by 0x40D5CF: bridge_destroy (bridge.c:3179)
==10653==by 0x409C2D: add_del_bridges (bridge.c:1698)
==10653==by 0x406CFA: bridge_reconfigure (bridge.c:582)
==10653==by 0x40CC4A: bridge_run (bridge.c:2950)
==10653==by 0x412818: main (ovs-vswitchd.c:116)
Backtrace reported by gdb using core resulting from make check:
at ./lib/list.h:257
257 return list->next == list;
(gdb) bt
at ./lib/list.h:257
at ofproto/connmgr.c:2261
fm=0x7f48936757e8, req=0x0) at ofproto/ofproto.c:4942
at ofproto/ofproto.c:6171
at ofproto/ofproto.c:6194
at ofproto/ofproto.c:1485
at lib/ovs-thread.c:337
at pthread_create.c:309
at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
CGLAGS=-g
gcc (Debian 4.9.2-10) 4.9.2
Signed-off-by: Simon Horman
---
ofproto/connmgr.c | 6 ++
tests/ofproto.at | 13 +
2 files changed, 19 insertions(+)
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index 975ee335b9f6..0a662f6b4074 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -273,6 +273,8 @@ connmgr_destroy(struct connmgr *mgr)
LIST_FOR_EACH_SAFE (ofconn, next_ofconn, node, &mgr->all_conns) {
ofconn_destroy(ofconn);
}
+
+mgr->ofproto->connmgr = NULL; /* Its going away! */
ovs_mutex_unlock(&ofproto_mutex);
hmap_destroy(&mgr->controllers);
@@ -2294,6 +2296,10 @@ ofmonitor_flush(struct connmgr *mgr)
{
struct ofconn *ofconn;
+if (!mgr) {
+return;
+}
+
LIST_FOR_EACH (ofconn, node, &mgr->all_conns) {
struct ofpbuf *msg;
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 9c5f0bb4e530..fef315f0e968 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -679,6 +679,19 @@ OFPST_GROUP reply (OF1.5):
OVS_VSWITCHD_STOP
AT_CLEANUP
+dnl This is really bare-bones.
+dnl It at least checks request and reply serialization and deserialization.
+AT_SETUP([ofproto - group add then bridge delete (OpenFlow 1.3)])
+OVS_VSWITCHD_START
+AT_DATA([groups.txt], [dnl
+group_id=1234,type=all,bucket=output:10
+group_id=1235,type=all,bucket=output:10
+])
+AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn add-groups br0 groups.txt])
+AT_CHECK([ovs-vsctl --if-exist del-br br0])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto - mod-port (OpenFlow 1.0)])
OVS_VSWITCHD_START
for command_config_state in \
--
2.1.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
