Re: [ovs-dev] [PATCH v1] ofproto: Fix OVS crash when packets hit Openflow rules with certain combinations of nested actions

[Anil Kumar Koli via dev]

Hi Ben,

Please consider this as a gentle remainder and provide your inputs on the 
below issue.

Thanks & Regards,
Anil Kumar

-Original Message-
From: Anil Kumar Koli 
Sent: Thursday, 5 September, 2019 11:12 PM
To: 'Ben Pfaff' 
Cc: 'Ilya Maximets' ; 'ovs-dev@openvswitch.org' 

Subject: RE: [ovs-dev] [PATCH v1] ofproto: Fix OVS crash when packets hit 
Openflow rules with certain combinations of nested actions

Hi Ben,

Please consider this as a gentle remainder and provide your inputs on the
below issue.
This issue has been discussed earlier in the following thread.
https://mail.openvswitch.org/pipermail/ovs-discuss/2019-April/048494.html

Best Regards,
Anil Kumar.
-Original Message-
From: Anil Kumar Koli 
Sent: Tuesday, 3 September, 2019 11:35 AM
To: 'Ilya Maximets' ; 'ovs-dev@openvswitch.org'

Cc: 'Ben Pfaff' 
Subject: RE: [ovs-dev] [PATCH v1] ofproto: Fix OVS crash when packets hit
Openflow rules with certain combinations of nested actions

Hi Ilya,

Sorry for late reply and thanks for the review comments.
Yes, the issue happens only in the userspace datapath and not in kernel
datapath. I have tested it on kernel datapath and could not see this issue for
the same build.

Regarding the suggested solution, I still feel like going with recursive mutex
approach is more cleaner then releasing the lock for
ofproto_packet_out_finish().  We have the ofproto_dpif_xcache_execute() which
may lead to race when a flow mod learn is called from another thread. May be
it could also be possible that in certain combination we may result the crash
in ofproto_packet_out_start().  May be Ben can share his inputs as well.

Best Regards,Anil
Kumar

-Original Message-
From: Ilya Maximets 
Sent: Thursday, 29 August, 2019 05:10 PM
To: Anil Kumar Koli ;
ovs-dev@openvswitch.org
Cc: 'Ben Pfaff' 
Subject: Re: [ovs-dev] [PATCH v1] ofproto: Fix OVS crash when packets hit
Openflow rules with certain combinations of nested actions

On 29.08.2019 12:52, Anil Kumar Koli wrote:
> Hi Ilya,
>
> Thanks for the review.
> Please find below the stack trace of the crash
>
>  (gdb) bt
> #0  0x7f0a3da46c37 in __GI_raise (sig=sig@entry=6) at
> ../nptl/sysdeps/unix/sysv/linux/raise.c:56
> #1  0x7f0a3da4a028 in __GI_abort () at abort.c:89
> #2  0x0094262e in ovs_abort_valist (err_no=,
> format=, args=args@entry=0x7fffaf59e308) at
> lib/util.c:419
> #3  0x009426b7 in ovs_abort (err_no=,
> format=format@entry=0xb0289f "%s: pthread_%s_%s failed") at
> lib/util.c:411
> #4  0x009104f9 in ovs_mutex_lock_at (l_=l_@entry=0xe47cc0
> , where=where@entry=0xad4199 "ofproto/ofproto.c:5391")
> at lib/ovs-thread.c:75
> #5  0x0083fb1f in ofproto_flow_mod_learn
> (ofm=ofm@entry=0x7fffaf59e620, keep_ref=,
> limit=, below_limitp=below_limitp@entry=0x7fffaf59e540)
> at ofproto/ofproto.c:5391
> #6  0x0085e5d0 in xlate_learn_action
> (ctx=ctx@entry=0x7fffaf5a02e0, learn=learn@entry=0x2b18308) at
> ofproto/ofproto-dpif-xlate.c:5378
> #7  0x008615c3 in do_xlate_actions (ofpacts=,
> ofpacts_len=, ctx=,
> is_last_action=, group_bucket_action=)
> at ofproto/ofproto-dpif-xlate.c:6893
> #8  0x0085edba in xlate_recursively (actions_xlator=0x860730
> , is_last_action=false, deepens=,
> rule=0x2b8e440, ctx=0x7fffaf5a02e0) at
> ofproto/ofproto-dpif-xlate.c:4233
> #9  xlate_table_action (ctx=0x7fffaf5a02e0, in_port=,
> table_id=, may_packet_in=,
> honor_table_miss=, with_ct_orig=,
> is_last_action=false,
> xlator=0x860730 ) at
> ofproto/ofproto-dpif-xlate.c:4361
> #10 0x00861aaa in xlate_ofpact_resubmit (resubmit= out>, resubmit=, resubmit=,
> is_last_action=, ctx=0x7fffaf5a02e0) at
> ofproto/ofproto-dpif-xlate.c:4672
> #11 do_xlate_actions (ofpacts=ofpacts@entry=0x2b654e8,
> ofpacts_len=ofpacts_len@entry=48, ctx=ctx@entry=0x7fffaf5a02e0,
> is_last_action=is_last_action@entry=true,
> group_bucket_action=group_bucket_action@entry=false)
> at ofproto/ofproto-dpif-xlate.c:6773
> #12 0x008696d6 in xlate_actions (xin=xin@entry=0x7fffaf5a0da0,
> xout=xout@entry=0x7fffaf5a11b0) at ofproto/ofproto-dpif-xlate.c:7570
> #13 0x00859b56 in upcall_xlate (wc=0x7fffaf5a23f0,
> odp_actions=0x7fffaf5a1bc0, upcall=0x7fffaf5a1150, udpif=0x2b10670) at
> ofproto/ofproto-dpif-upcall.c:1197
> #14 process_upcall (udpif=udpif@entry=0x2b10670,
> upcall=upcall@entry=0x7fffaf5a1150,
> odp_actions=odp_actions@entry=0x7fffaf5a1bc0,
> wc=wc@entry=0x7fffaf5a23f0) at ofproto/ofproto-dpif-upcall.c:1413
> #15 0x0085a9eb in upcall_cb (packet=,
> flow=0x7fffaf5a2150, ufid=, pmd_id=,
> type=, userdata=, actions=0x7fffaf5a1bc0,
> wc=0x7fffaf5a23f0,
> put_actions=0x7fffaf5a1c00, aux=0x2b10670) at
> ofproto/ofproto-dpif-upcall.c:1315
> #16 0x0088419c in dp_netdev_upcall (pmd=pmd@entry=0x7f0a35f34010,
> packet_=packet_@entry=0x2b68930, flow=flow@entry=0x7fffaf5a2150,
> wc=wc@entry=0x7fffaf5a23f0, ufid=ufid@entry=0x7fffaf5a1ba0,
> type=type@entry=DPIF_UC_MISS,
> 

Re: [ovs-dev] [PATCH v5 1/9] ovs-vswitchd: Add Datapath, CT_Zone, and CT_Zone_Policy tables.

[Justin Pettit]

> On Aug 28, 2019, at 3:14 PM, Yi-Hung Wei  wrote:
> 
> From: Justin Pettit 
> 
> Signed-off-by: Justin Pettit 
> Signed-off-by: Yi-Hung Wei 
> Co-authored-by: Yi-Hung Wei 

Thanks.  I merged this into master.

--Justin


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

Re: [ovs-dev] [PATCH v3 ovn 0/2] ovn-controller: Logical flow processing optimizations

[Han Zhou]

On Thu, Sep 12, 2019 at 7:14 AM Dumitru Ceara  wrote:
>
> This series adds some (independent) optimizations that improve
> ovn-controller performance by lowering the number of operations that
> need to be executed during a iteration of the main controller loop.
>
> Each patch in the series addresses a bottleneck reported by running
> `perf record -g --call-graph dwarf -i /usr/bin/ovn-controller` on a
> system where the Southbound DB was already populated by ovn-northd.
>
> The logical configuration of the OVN is:
> - 500 logical routers
> - 1 logical switch attached to each router
> - 2 logical ports attached to each switch
> - 4 ACLs per switch
>
> To evaluate the performance impact of the change in a more realistic
> way the following test is performed (on a single node that runs ovn-northd
> and ovn-controller and emulates the VM hosts with OVS internal
interfaces):
> 1. Start ovs-vswitchd.
> 2. Remove the Southbound DB such that ovn-northd needs to repopulate
>everything based on the Northbound DB contents.
> 3. Start a patched version of ovn-controller such that it tracks loop time
>execution information and how long it takes to process all logical
flows
>from the Southbound DB.
> 4. At the same time with step 3 above, start ovn-northd which will
populate
>the Southbound DB.
>
> The following measurements are taken:
> - real time (measured with `time`) to have all corresponding openflow
flows
>   processed by ovs-vswitchd.
> - maximum ovn-controller main loop iteration duration
> - average ovn-controller main loop iteration duration
> - number of times ovn-controller executes the main loop until all logical
>   flows are processed
>
> Comparing the results before and after this series we see:
> - 13% performance boost w.r.t. real time taken to have all openflow flows
>   installed in ovs-vswitchd
> - 41% decrease of average ovn-controller loop iteration duration and
>   12% increase of ovn-controller number of loop iterations. Essentially
>   processing loops are faster and can happen more often.
> - 2% decrease of maximum ovn-controller main loop iteration duration.
>
>
> Dumitru Ceara (2):
>   ovn-controller: Optimize update of ct-zones external-ids.
>   ovn-controller: Minimize SB DB port_binding lookups.
>
>
>  controller/binding.c|   19 ---
>  controller/ovn-controller.c |   53
+--
>  controller/ovn-controller.h |   11 -
>  controller/physical.c   |   17 --
>  controller/pinctrl.c|   53
+++
>  5 files changed, 93 insertions(+), 60 deletions(-)
>
>
> ---
> v3:
>   - Remove first patch from the series as it was already applied.
>   - Rebase rest of series.
> v2: Send series for OVN repo instead of OVS.
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Thanks Dumitru. For the series:

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

Re: [ovs-dev] Querying OVS for OpenFlow capabilities

[Justin Pettit]

> On Sep 12, 2019, at 2:15 PM, William Tu  wrote:
> 
> On Thu, Sep 12, 2019 at 03:47:58PM -0400, Mark Michelson wrote:
>> On 9/12/19 2:46 PM, Justin Pettit wrote:
>>> 
>>> 
 On Sep 12, 2019, at 11:08 AM, Mark Michelson  wrote:
 
 Hi guys,
 
 I recently sent a new version of a document describing proposed OVN/OVS 
 compatibility [1]. In it, I suggested that when we add new OpenFlow 
 capabilities to OVS, then OVN needs to probe OVS for the OpenFlow 
 capabilities at startup. This way we can know whether we have the 
 capability, can log an informational message, and can just generally be as 
 graceful as possible.
 
 Numan brought up that the ability to probe at runtime for an OpenFlow 
 feature may not exist yet. I'm aware right now that we could attempt to 
 program a flow that uses the capability we're interested in and see if it 
 succeeds. But I was wondering if there's a more formal way of doing it.
>>> 
>>> Thanks for writing up that document.  I do think probing is the best way to 
>>> do it.  We do similar feature probing of the datapath in ovs-vswitchd (see 
>>> dpif_probe_feature() in lib/dpif.c).
>>> 
>>> This brings up another wrinkle: datapath compatibility.  Feature support 
>>> changes depending on the datapath, and there's starting to be a 
>>> proliferation of datapaths (upstream Linux kernel datapath, OVS Linux 
>>> kernel datapath, Windows datapath, and userspace datapath).  These have 
>>> different features that can affect the ability to make OVS work as the 
>>> controller expects.
>>> 
>>> William (cc'd) is looking at adding a new feature compatibility column to 
>>> the Datapath table that should be landing in master this week.  This will 
>>> allow a controller to probe for datapath-level features.  (And some of 
>>> those could probably be proxies for OpenFlow-level support for features.)  
>>> I'm not sure if he's started coding it yet, though.
>> 
>> I think this could cover us. The big thing we want to be able to do is to
>> probe for the existence of a runtime feature. If the datapath determines the
>> existence of that feature, then being able to query the datapath for the
>> feature makes sense.
> 
> I've been thinking about doing it for a while but not yet started.
> Glad to know it's useful, I will start working on it this week.

I just pushed the database changes so it doesn't block you.  I hope to get the 
rest of that timeout policy series pushed today or tomorrow.

--Justin


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

Re: [ovs-dev] Querying OVS for OpenFlow capabilities

[William Tu]

On Thu, Sep 12, 2019 at 03:47:58PM -0400, Mark Michelson wrote:
> On 9/12/19 2:46 PM, Justin Pettit wrote:
> >
> >
> >>On Sep 12, 2019, at 11:08 AM, Mark Michelson  wrote:
> >>
> >>Hi guys,
> >>
> >>I recently sent a new version of a document describing proposed OVN/OVS 
> >>compatibility [1]. In it, I suggested that when we add new OpenFlow 
> >>capabilities to OVS, then OVN needs to probe OVS for the OpenFlow 
> >>capabilities at startup. This way we can know whether we have the 
> >>capability, can log an informational message, and can just generally be as 
> >>graceful as possible.
> >>
> >>Numan brought up that the ability to probe at runtime for an OpenFlow 
> >>feature may not exist yet. I'm aware right now that we could attempt to 
> >>program a flow that uses the capability we're interested in and see if it 
> >>succeeds. But I was wondering if there's a more formal way of doing it.
> >
> >Thanks for writing up that document.  I do think probing is the best way to 
> >do it.  We do similar feature probing of the datapath in ovs-vswitchd (see 
> >dpif_probe_feature() in lib/dpif.c).
> >
> >This brings up another wrinkle: datapath compatibility.  Feature support 
> >changes depending on the datapath, and there's starting to be a 
> >proliferation of datapaths (upstream Linux kernel datapath, OVS Linux kernel 
> >datapath, Windows datapath, and userspace datapath).  These have different 
> >features that can affect the ability to make OVS work as the controller 
> >expects.
> >
> >William (cc'd) is looking at adding a new feature compatibility column to 
> >the Datapath table that should be landing in master this week.  This will 
> >allow a controller to probe for datapath-level features.  (And some of those 
> >could probably be proxies for OpenFlow-level support for features.)  I'm not 
> >sure if he's started coding it yet, though.
> 
> I think this could cover us. The big thing we want to be able to do is to
> probe for the existence of a runtime feature. If the datapath determines the
> existence of that feature, then being able to query the datapath for the
> feature makes sense.

I've been thinking about doing it for a while but not yet started.
Glad to know it's useful, I will start working on it this week.

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

Re: [ovs-dev] [RFC v2] Document process for compatibility between OVS and OVN.

[Han Zhou]

On Thu, Sep 12, 2019 at 2:00 PM Mark Michelson  wrote:
>
> On 9/12/19 4:51 PM, Han Zhou wrote:
> > Thanks Mark for writing this up.
> >
> > On Fri, Sep 6, 2019 at 2:08 PM Mark Michelson  > > wrote:
> >  >
> >  > This document serves to provide an explanation for how OVN will
remain
> >  > compatible with OVS. It provides instructions for OVN contributors
for
> >  > how to maintain compatibility even across older versions of OVS when
> >  > possible.
> >  >
> >  > Note that the document currently makes reference to some non-existent
> >  > items. For instance, it refers to an ovs-compat directory in the OVN
> >  > project. Also, it refers to some macros for testing the OVS version.
> >  > These will be added in a future commit if the process documented
here is
> >  > approved.
> >  >
> >  > I'm submitting this as an RFC, since I imagine there are some
details I
> >  > have not thought about, and there will also be some great suggestions
> >  > from others on this matter.
> >  >
> >  > Signed-off-by: Mark Michelson  > >
> >  > ---
> >  >  Documentation/automake.mk 

> > |   1 +
> >  >  Documentation/internals/contributing/index.rst |   1 +
> >  >  .../contributing/ovs-ovn-compatibility.rst | 129
> > +
> >  >  3 files changed, 131 insertions(+)
> >  >  create mode 100644
> > Documentation/internals/contributing/ovs-ovn-compatibility.rst
> >  >
> >  > diff --git a/Documentation/automake.mk 
> > b/Documentation/automake.mk 
> >  > index f7e1d2628..d9bd4be1f 100644
> >  > --- a/Documentation/automake.mk 
> >  > +++ b/Documentation/automake.mk 
> >  > @@ -56,6 +56,7 @@ DOC_SOURCE = \
> >  > Documentation/internals/contributing/documentation-style.rst
\
> >  > Documentation/internals/contributing/libopenvswitch-abi.rst \
> >  > Documentation/internals/contributing/submitting-patches.rst \
> >  > +
Documentation/internals/contributing/ovs-ovn-compatibility.rst \
> >  > Documentation/requirements.txt \
> >  > $(addprefix Documentation/ref/,$(RST_MANPAGES)
> > $(RST_MANPAGES_NOINST))
> >  >  FLAKE8_PYFILES += Documentation/conf.py
> >  > diff --git a/Documentation/internals/contributing/index.rst
> > b/Documentation/internals/contributing/index.rst
> >  > index a46cb046a..7ef57a1e2 100644
> >  > --- a/Documentation/internals/contributing/index.rst
> >  > +++ b/Documentation/internals/contributing/index.rst
> >  > @@ -36,3 +36,4 @@ The below guides provide information on
> > contributing to Open vSwitch itself.
> >  > coding-style-windows
> >  > documentation-style
> >  > libopenvswitch-abi
> >  > +   ovs-ovn-compatibility
> >  > diff --git
> > a/Documentation/internals/contributing/ovs-ovn-compatibility.rst
> > b/Documentation/internals/contributing/ovs-ovn-compatibility.rst
> >  > new file mode 100644
> >  > index 0..d40b58c32
> >  > --- /dev/null
> >  > +++ b/Documentation/internals/contributing/ovs-ovn-compatibility.rst
> >  > @@ -0,0 +1,129 @@
> >  > +..
> >  > +  Copyright (c) 2019 Nicira, Inc.
> >  > +
> >  > +  Licensed under the Apache License, Version 2.0 (the
> > "License"); you may
> >  > +  not use this file except in compliance with the License. You
> > may obtain
> >  > +  a copy of the License at
> >  > +
> >  > + http://www.apache.org/licenses/LICENSE-2.0
> >  > +
> >  > +  Unless required by applicable law or agreed to in writing,
> > software
> >  > +  distributed under the License is distributed on an "AS IS"
> > BASIS, WITHOUT
> >  > +  WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> > implied. See the
> >  > +  License for the specific language governing permissions and
> > limitations
> >  > +  under the License.
> >  > +
> >  > +  Convention for heading levels in Open vSwitch documentation:
> >  > +
> >  > +  ===  Heading 0 (reserved for the title in a document)
> >  > +  ---  Heading 1
> >  > +  ~~~  Heading 2
> >  > +  +++  Heading 3
> >  > +  '''  Heading 4
> >  > +
> >  > +  Avoid deeper levels because they do not render well.
> >  > +
> >  > +===
> >  > +Keeping OVN Compatible with OVS
> >  > +===
> >  > +
> >  > +OVN has split from OVS. Prior to this split, there was no issue with
> >  > +compatibility. All code changes happened at the same time and in the
> > same repo.
> >  > +New releases contained the latest OVS and OVN changes. But now these
> > assumptions
> >  > +are no longer valid, and so care must be taken to maintain
> > compatibility.
> >  > +
> >  > +OVS and OVN versions
> >  > +
> >  > +
> >  > +Prior to the split, the release schedule for OVN was bound to the
> > release
> >  > +schedule for OVS. OVS releases a new version approximately every 6
> > months. 

Re: [ovs-dev] [PATCH v5 2/9] ovs-vsctl: Add conntrack zone commands.

[Justin Pettit]

> On Aug 28, 2019, at 3:14 PM, Yi-Hung Wei  wrote:
> 
> diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> index 7c09df79bd29..5b9883ae1c3d 100644
> --- a/utilities/ovs-vsctl.8.in
> +++ b/utilities/ovs-vsctl.8.in
> @@ -353,6 +353,32 @@ list.
> Prints the name of the bridge that contains \fIiface\fR on standard
> output.
> .
> +.SS "Conntrack Zone Commands"
> ...
> +Without \fB\-\-may\-exist\fR, attempting to add a \fIzone_id\fR that
> +already exists is an error.  With \fB\-\-may\-exist\fR,
> +this command does nothing if \fIzone_id\fR is already created\fR.

I don't think you need that final \fR.

I also made a few minor language tweaks in the icremental.

> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> index 4948137efe8c..76a708bd5069 100644
> --- a/utilities/ovs-vsctl.c
> +++ b/utilities/ovs-vsctl.c
> 
> +static struct ovsrec_ct_timeout_policy *
> +create_timeout_policy(struct ctl_context *ctx, char **argv, int n_tps)
> +{


I think it's clearer if we switch "argv" to "tps", since the argument should 
only be timeout policies.

> +static void
> +cmd_list_zone_tp(struct ctl_context *ctx)
> +{
> ...
> +for (int j = 0; j < tp->n_timeouts; j++) {
> +if (j == tp->n_timeouts - 1) {
> +ds_put_format(>output, "%s=%"PRIu64"\n",
> +  tp->key_timeouts[j], tp->value_timeouts[j]);
> +} else {
> +ds_put_format(>output, "%s=%"PRIu64" ",
> +  tp->key_timeouts[j], tp->value_timeouts[j]);
> +}

I think this can be done without the duplicated code with a ds_chomp() and 
ds_put_char().  Let me know if you agree with the incremental patch at the 
bottom.

> static void
> cmd_add_br(struct ctl_context *ctx)
> {
> @@ -2896,6 +3083,13 @@ static const struct ctl_command_syntax 
> vsctl_commands[] = {
> /* Switch commands. */
> {"emer-reset", 0, 0, "", pre_cmd_emer_reset, cmd_emer_reset, NULL, "", 
> RW},
> 
> +/* Zone and CT Timeout Policy commands. */
> +{"add-zone-tp", 2, 18, "", pre_get_zone, cmd_add_zone_tp, NULL,
> + "--may-exist", RW},

Is there a reason not to make the minimum arguments 3 instead of 2?  It seems 
like it's required in the code.

Is there a reason you limited this to 18 arguments and not use INT_MAX?

Let me know if you agree with the incremental.

Thanks,

--Justin


-=-=-=-=-=-=-=-=-=-=-

diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
index 5b9883ae1c3d..14a8aa4a48ac 100644
--- a/utilities/ovs-vsctl.8.in
+++ b/utilities/ovs-vsctl.8.in
@@ -361,13 +361,13 @@ Creates a conntrack zone timeout policy with 
\fIzone_id\fR in
 \fIdatapath\fR.  The \fIpolicies\fR consist of \fIkey\fB=\fIvalue\fR
 pairs, separated by spaces.  For example, \fBicmp_first=30
 icmp_reply=60\fR specifies a 30-second timeout policy for the first ICMP
-packet and a 60-second policy for ICMP reply packet.  See the
+packet and a 60-second policy for ICMP reply packets.  See the
 \fBCT_Timeout_Policy\fR table in \fBovs-vswitchd.conf.db\fR(5) for the
 supported keys.
 .IP
 Without \fB\-\-may\-exist\fR, attempting to add a \fIzone_id\fR that
 already exists is an error.  With \fB\-\-may\-exist\fR,
-this command does nothing if \fIzone_id\fR is already created\fR.
+this command does nothing if \fIzone_id\fR already exists.
 .
 .IP "[\fB\-\-if\-exists\fR] \fBdel\-zone\-tp \fIdatapath \fBzone=\fIzone_id\fR"
 Delete the timeout policy associated with \fIzone_id\fR from \fIdatapath\fR.
diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
index 76a708bd5069..43f64d4711f9 100644
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -1180,7 +1180,7 @@ find_ct_zone(struct ovsrec_datapath *dp, const int64_t 
zone_id)
 }
 
 static struct ovsrec_ct_timeout_policy *
-create_timeout_policy(struct ctl_context *ctx, char **argv, int n_tps)
+create_timeout_policy(struct ctl_context *ctx, char **tps, int n_tps)
 {
 const struct ovsrec_ct_timeout_policy_table *tp_table;
 const struct ovsrec_ct_timeout_policy *row;
@@ -1193,7 +1193,7 @@ create_timeout_policy(struct ctl_context *ctx, char 
**argv, int n_tps)
 
 /* Parse timeout arguments. */
 for (int i = 0; i < n_tps; i++) {
-policies[i] = xstrdup(argv[i]);
+policies[i] = xstrdup(tps[i]);
 
 char *key, *value;
 char *policy = policies[i];
@@ -1320,14 +1320,11 @@ cmd_list_zone_tp(struct ctl_context *ctx)
 struct ovsrec_ct_timeout_policy *tp = zone->timeout_policy;
 
 for (int j = 0; j < tp->n_timeouts; j++) {
-if (j == tp->n_timeouts - 1) {
-ds_put_format(>output, "%s=%"PRIu64"\n",
+ds_put_format(>output, "%s=%"PRIu64" ",
   tp->key_timeouts[j], tp->value_timeouts[j]);
-} else {
-ds_put_format(>output, "%s=%"PRIu64" ",
-  tp->key_timeouts[j], tp->value_timeouts[j]);
-}
 }
+ds_chomp(>output, ' ');
+

Re: [ovs-dev] [RFC v2] Document process for compatibility between OVS and OVN.

[Mark Michelson]

On 9/12/19 4:51 PM, Han Zhou wrote:

Thanks Mark for writing this up.

On Fri, Sep 6, 2019 at 2:08 PM Mark Michelson > wrote:

 >
 > This document serves to provide an explanation for how OVN will remain
 > compatible with OVS. It provides instructions for OVN contributors for
 > how to maintain compatibility even across older versions of OVS when
 > possible.
 >
 > Note that the document currently makes reference to some non-existent
 > items. For instance, it refers to an ovs-compat directory in the OVN
 > project. Also, it refers to some macros for testing the OVS version.
 > These will be added in a future commit if the process documented here is
 > approved.
 >
 > I'm submitting this as an RFC, since I imagine there are some details I
 > have not thought about, and there will also be some great suggestions
 > from others on this matter.
 >
 > Signed-off-by: Mark Michelson >

 > ---
 >  Documentation/automake.mk    
    |   1 +

 >  Documentation/internals/contributing/index.rst     |   1 +
 >  .../contributing/ovs-ovn-compatibility.rst         | 129 
+

 >  3 files changed, 131 insertions(+)
 >  create mode 100644 
Documentation/internals/contributing/ovs-ovn-compatibility.rst

 >
 > diff --git a/Documentation/automake.mk  
b/Documentation/automake.mk 

 > index f7e1d2628..d9bd4be1f 100644
 > --- a/Documentation/automake.mk 
 > +++ b/Documentation/automake.mk 
 > @@ -56,6 +56,7 @@ DOC_SOURCE = \
 >         Documentation/internals/contributing/documentation-style.rst \
 >         Documentation/internals/contributing/libopenvswitch-abi.rst \
 >         Documentation/internals/contributing/submitting-patches.rst \
 > +       Documentation/internals/contributing/ovs-ovn-compatibility.rst \
 >         Documentation/requirements.txt \
 >         $(addprefix Documentation/ref/,$(RST_MANPAGES) 
$(RST_MANPAGES_NOINST))

 >  FLAKE8_PYFILES += Documentation/conf.py
 > diff --git a/Documentation/internals/contributing/index.rst 
b/Documentation/internals/contributing/index.rst

 > index a46cb046a..7ef57a1e2 100644
 > --- a/Documentation/internals/contributing/index.rst
 > +++ b/Documentation/internals/contributing/index.rst
 > @@ -36,3 +36,4 @@ The below guides provide information on 
contributing to Open vSwitch itself.

 >     coding-style-windows
 >     documentation-style
 >     libopenvswitch-abi
 > +   ovs-ovn-compatibility
 > diff --git 
a/Documentation/internals/contributing/ovs-ovn-compatibility.rst 
b/Documentation/internals/contributing/ovs-ovn-compatibility.rst

 > new file mode 100644
 > index 0..d40b58c32
 > --- /dev/null
 > +++ b/Documentation/internals/contributing/ovs-ovn-compatibility.rst
 > @@ -0,0 +1,129 @@
 > +..
 > +      Copyright (c) 2019 Nicira, Inc.
 > +
 > +      Licensed under the Apache License, Version 2.0 (the 
"License"); you may
 > +      not use this file except in compliance with the License. You 
may obtain

 > +      a copy of the License at
 > +
 > + http://www.apache.org/licenses/LICENSE-2.0
 > +
 > +      Unless required by applicable law or agreed to in writing, 
software
 > +      distributed under the License is distributed on an "AS IS" 
BASIS, WITHOUT
 > +      WARRANTIES OR CONDITIONS OF ANY KIND, either express or 
implied. See the
 > +      License for the specific language governing permissions and 
limitations

 > +      under the License.
 > +
 > +      Convention for heading levels in Open vSwitch documentation:
 > +
 > +      ===  Heading 0 (reserved for the title in a document)
 > +      ---  Heading 1
 > +      ~~~  Heading 2
 > +      +++  Heading 3
 > +      '''  Heading 4
 > +
 > +      Avoid deeper levels because they do not render well.
 > +
 > +===
 > +Keeping OVN Compatible with OVS
 > +===
 > +
 > +OVN has split from OVS. Prior to this split, there was no issue with
 > +compatibility. All code changes happened at the same time and in the 
same repo.
 > +New releases contained the latest OVS and OVN changes. But now these 
assumptions
 > +are no longer valid, and so care must be taken to maintain 
compatibility.

 > +
 > +OVS and OVN versions
 > +
 > +
 > +Prior to the split, the release schedule for OVN was bound to the 
release
 > +schedule for OVS. OVS releases a new version approximately every 6 
months. OVN
 > +has not yet determined a release schedule, but it is entirely 
possible that it
 > +will be different from OVS. Eventually, this will lead to a 
situation where it
 > +is very important that we publish which versions of OVN are 
compatible with
 > +which versions of OVS. When incompatibilities are discovered, it is 
important to

 > +ensure that these are clearly stated.
 > +
 > +The split of OVS and OVN happened in the run-up to the release of 

Re: [ovs-dev] [PATCH 05/10] trigger: Free leaked ovsdb_schema

[aginwala]

One minor suggestion here:
Can you also handle freeing result:
diff --git a/ovsdb/trigger.c b/ovsdb/trigger.c
index 6f4ed96b0..0158957d6 100644
--- a/ovsdb/trigger.c
+++ b/ovsdb/trigger.c
@@ -214,6 +214,7 @@ ovsdb_trigger_try(struct ovsdb_trigger *t, long long
int now)
 /* Unsatisfied "wait" condition.  Take no action now,
retry
  * later. */
 }
+json_destroy(result);
 return false;
 }

Else I can handle that in separate patch. Else, acked by for the series.
Acked-by: Aliasgar Ginwala 

On Wed, Sep 11, 2019 at 2:19 PM Yifeng Sun  wrote:

> Valgrind reported:
>
> 1925: schema conversion online - standalone
>
> ==10884== 689 (56 direct, 633 indirect) bytes in 1 blocks are definitely
> lost in loss record 384 of 420
> ==10884==at 0x4C2FB55: calloc (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==10884==by 0x44A592: xcalloc (util.c:121)
> ==10884==by 0x40E2EC: ovsdb_schema_create (ovsdb.c:41)
> ==10884==by 0x40E688: ovsdb_schema_from_json (ovsdb.c:217)
> ==10884==by 0x416C6F: ovsdb_trigger_try (trigger.c:246)
> ==10884==by 0x40D4DE: ovsdb_jsonrpc_trigger_create
> (jsonrpc-server.c:1119)
> ==10884==by 0x40D4DE: ovsdb_jsonrpc_session_got_request
> (jsonrpc-server.c:986)
> ==10884==by 0x40D4DE: ovsdb_jsonrpc_session_run (jsonrpc-server.c:556)
> ==10884==by 0x40D4DE: ovsdb_jsonrpc_session_run_all
> (jsonrpc-server.c:586)
> ==10884==by 0x40D4DE: ovsdb_jsonrpc_server_run (jsonrpc-server.c:401)
> ==10884==by 0x406A6E: main_loop (ovsdb-server.c:209)
> ==10884==by 0x406A6E: main (ovsdb-server.c:460)
>
> 'new_schema' should also be freed when there is no error.
> This patch fixes it.
>
> Signed-off-by: Yifeng Sun 
> ---
>  ovsdb/trigger.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ovsdb/trigger.c b/ovsdb/trigger.c
> index 6f4ed96b000b..7e62e90ae381 100644
> --- a/ovsdb/trigger.c
> +++ b/ovsdb/trigger.c
> @@ -254,8 +254,8 @@ ovsdb_trigger_try(struct ovsdb_trigger *t, long long
> int now)
>  if (!error) {
>  error = ovsdb_convert(t->db, new_schema, );
>  }
> +ovsdb_schema_destroy(new_schema);
>  if (error) {
> -ovsdb_schema_destroy(new_schema);
>  trigger_convert_error(t, error);
>  return false;
>  }
> --
> 2.7.4
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC v2] Document process for compatibility between OVS and OVN.

[Ben Pfaff]

On Thu, Sep 12, 2019 at 04:06:14PM -0400, Mark Michelson wrote:
> On 9/12/19 3:25 PM, Ben Pfaff wrote:
> > It's possible that we could introduce features for logical flows that
> > allow different behavior based on whether a given node supports a
> > particular feature.  I've thought about this in the past, but it
> > didn't become necessary yet.
> 
> I'm curious how this would operate. I tend to think of logical flows as
> describing a feature at a high level. It's then up to ovn-controller to
> translate this as appropriate. So I would think the logical flow would not
> be interested in what the node's datapath supports, since the node is free
> to translate the logical flow as it sees fit.
> 
> I guess this would be useful if the presence of a feature on a node might
> affect subsequent logical flows beyond the current one being evaluated,
> though.

The kind of compatibility I have in mind would cover two kinds of
situations:

1. If ovn-northd has to deal with cases where ovn-controller might not
   be up-to-date on some nodes, then it might need to enable some
   features conditionally.  In the upgrade order we originally
   prescribed, this will not happen because ovn-controller is upgraded
   first, but I remember hearing that there is some kind of pressure
   to support upgrading in the opposite order.

2. If ovn-controller has to deal with cases where OVS or its datapath
   might not be up-to-date in some important way, and there's some
   kind of fallback that can be implemented for older versions.

I envision this as being the moral equivalent of #ifdefs for logical
flows.

It's better if it's not needed.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC v2] Document process for compatibility between OVS and OVN.

[Han Zhou]

Thanks Mark for writing this up.

On Fri, Sep 6, 2019 at 2:08 PM Mark Michelson  wrote:
>
> This document serves to provide an explanation for how OVN will remain
> compatible with OVS. It provides instructions for OVN contributors for
> how to maintain compatibility even across older versions of OVS when
> possible.
>
> Note that the document currently makes reference to some non-existent
> items. For instance, it refers to an ovs-compat directory in the OVN
> project. Also, it refers to some macros for testing the OVS version.
> These will be added in a future commit if the process documented here is
> approved.
>
> I'm submitting this as an RFC, since I imagine there are some details I
> have not thought about, and there will also be some great suggestions
> from others on this matter.
>
> Signed-off-by: Mark Michelson 
> ---
>  Documentation/automake.mk  |   1 +
>  Documentation/internals/contributing/index.rst |   1 +
>  .../contributing/ovs-ovn-compatibility.rst | 129
+
>  3 files changed, 131 insertions(+)
>  create mode 100644
Documentation/internals/contributing/ovs-ovn-compatibility.rst
>
> diff --git a/Documentation/automake.mk b/Documentation/automake.mk
> index f7e1d2628..d9bd4be1f 100644
> --- a/Documentation/automake.mk
> +++ b/Documentation/automake.mk
> @@ -56,6 +56,7 @@ DOC_SOURCE = \
> Documentation/internals/contributing/documentation-style.rst \
> Documentation/internals/contributing/libopenvswitch-abi.rst \
> Documentation/internals/contributing/submitting-patches.rst \
> +   Documentation/internals/contributing/ovs-ovn-compatibility.rst \
> Documentation/requirements.txt \
> $(addprefix Documentation/ref/,$(RST_MANPAGES)
$(RST_MANPAGES_NOINST))
>  FLAKE8_PYFILES += Documentation/conf.py
> diff --git a/Documentation/internals/contributing/index.rst
b/Documentation/internals/contributing/index.rst
> index a46cb046a..7ef57a1e2 100644
> --- a/Documentation/internals/contributing/index.rst
> +++ b/Documentation/internals/contributing/index.rst
> @@ -36,3 +36,4 @@ The below guides provide information on contributing to
Open vSwitch itself.
> coding-style-windows
> documentation-style
> libopenvswitch-abi
> +   ovs-ovn-compatibility
> diff --git
a/Documentation/internals/contributing/ovs-ovn-compatibility.rst
b/Documentation/internals/contributing/ovs-ovn-compatibility.rst
> new file mode 100644
> index 0..d40b58c32
> --- /dev/null
> +++ b/Documentation/internals/contributing/ovs-ovn-compatibility.rst
> @@ -0,0 +1,129 @@
> +..
> +  Copyright (c) 2019 Nicira, Inc.
> +
> +  Licensed under the Apache License, Version 2.0 (the "License");
you may
> +  not use this file except in compliance with the License. You may
obtain
> +  a copy of the License at
> +
> +  http://www.apache.org/licenses/LICENSE-2.0
> +
> +  Unless required by applicable law or agreed to in writing, software
> +  distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT
> +  WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the
> +  License for the specific language governing permissions and
limitations
> +  under the License.
> +
> +  Convention for heading levels in Open vSwitch documentation:
> +
> +  ===  Heading 0 (reserved for the title in a document)
> +  ---  Heading 1
> +  ~~~  Heading 2
> +  +++  Heading 3
> +  '''  Heading 4
> +
> +  Avoid deeper levels because they do not render well.
> +
> +===
> +Keeping OVN Compatible with OVS
> +===
> +
> +OVN has split from OVS. Prior to this split, there was no issue with
> +compatibility. All code changes happened at the same time and in the
same repo.
> +New releases contained the latest OVS and OVN changes. But now these
assumptions
> +are no longer valid, and so care must be taken to maintain compatibility.
> +
> +OVS and OVN versions
> +
> +
> +Prior to the split, the release schedule for OVN was bound to the release
> +schedule for OVS. OVS releases a new version approximately every 6
months. OVN
> +has not yet determined a release schedule, but it is entirely possible
that it
> +will be different from OVS. Eventually, this will lead to a situation
where it
> +is very important that we publish which versions of OVN are compatible
with
> +which versions of OVS. When incompatibilities are discovered, it is
important to
> +ensure that these are clearly stated.
> +
> +The split of OVS and OVN happened in the run-up to the release of OVS
2.12. As a
> +result, all versions of OVN *must* be compiled against OVS version 2.12
or
> +later. Before going further into compatibility, let's explore the ways
that OVN
> +and OVS can become incompatible.
> +
> +Compile-time Incompatibility
> +
> +
> +The first way that the projects can 

Re: [ovs-dev] [RFC v2] Document process for compatibility between OVS and OVN.

[Mark Michelson]

On 9/12/19 3:25 PM, Ben Pfaff wrote:

On Fri, Sep 06, 2019 at 05:08:17PM -0400, Mark Michelson wrote:

+===
+Keeping OVN Compatible with OVS
+===
+
+OVN has split from OVS. Prior to this split, there was no issue with
+compatibility. All code changes happened at the same time and in the same repo.
+New releases contained the latest OVS and OVN changes. But now these 
assumptions
+are no longer valid, and so care must be taken to maintain compatibility.


"no issue with compatibility" significantly overstates the case, since
a distributed system has to be upgraded in some order (and different
parties actually wanted different orders!).  I agree with the larger
point that the split increases the difficulty.


+Compile-time Incompatibility
+
+
+The first way that the projects can become incompatible is if the C code for 
OVN
+no longer can compile.
+
+The most likely case for this would be that an OVN change requires a parallel
+change to OVS. Those keeping up to date with OVN but not OVS will find that OVN
+will no longer compile since it refers to a nonexistent function or out of date
+function in OVS.
+
+Most OVN users will consume OVN via package from their distribution of choice.
+OVN consumes libopenvswitch statically, so even if the version of OVS installed
+on a user's machine is incompatible at compile time, it will not matter.
+
+OVN developers are the only ones that would be inconvenienced by a compile-time
+incompatibility. OVN developers will be expected to regularly update the 
version
+of OVS they are using. If an OVN developer notices that OVN is not compiling,
+then they should update their OVS code to the latest and try again.
+
+Developers who are making changes to both OVS and OVN at the same time *must*
+contribute the OVS change first and ensure it is merged upstream before
+submitting the OVN change. This way, OVN should never be in a state where it
+will not compile.
+
+When compiling older releases of OVN, it should be able to compile against 
newer
+versions of OVS due to API and ABI guarantees in OVS's libaries.


The above section doesn't observe the difference between minor and
major newer versions.  I believe that our current ABI/API guarantee is
for different minro versions within a major release.  It is not
necessarily the case that a newer major release has compatible ABI/API
with an older one.  (I think that we use library naming to make this
clear.)


+Runtime Incompatibility
+---
+
+The next way that the projects may become incompatible is at runtime. The most
+common way this would happen is if new OpenFlow capabilities are added to OVS 
as
+part of an OVN change. In this case, if someone updates OVN but does not also
+updage OVS, then OVN will not be able to install the OpenFlow rules it wishes
+to.
+
+Unlike with compile-time incompatibilities, we can't wallpaper over the fact
+that the OVS installation is not up to date. The best we can do is make it very
+clear at runtime that a certain feature is not present, and if the feature is
+desired, OVS must be upgraded.
+
+The following is the process that OVN developers should use when making a
+runtime compatibility change to OVS and OVN.
+
+1. Submit the change to OVS first. See the change through until it is merged.
+2. Make the necessary changes to OVN.
+
+  a. At startup, probe OVS for the existence of the OpenFlow addition. If it
+ is not present, then output an informational message that explains which
+ OVN feature(s) cannot be used.
+  b. If a user attempts to explicitly configure the feature that is not usable
+ due to the incompatibility, then output a warning message.
+  c. Ensure that the code that installs the OpenFlow will only do so if the new
+ feature is present.


It's possible that we could introduce features for logical flows that
allow different behavior based on whether a given node supports a
particular feature.  I've thought about this in the past, but it
didn't become necessary yet.



I'm curious how this would operate. I tend to think of logical flows as 
describing a feature at a high level. It's then up to ovn-controller to 
translate this as appropriate. So I would think the logical flow would 
not be interested in what the node's datapath supports, since the node 
is free to translate the logical flow as it sees fit.


I guess this would be useful if the presence of a feature on a node 
might affect subsequent logical flows beyond the current one being 
evaluated, though.

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

Re: [ovs-dev] [RFC v2] Document process for compatibility between OVS and OVN.

[Ben Pfaff]

On Fri, Sep 06, 2019 at 05:08:17PM -0400, Mark Michelson wrote:
> +===
> +Keeping OVN Compatible with OVS
> +===
> +
> +OVN has split from OVS. Prior to this split, there was no issue with
> +compatibility. All code changes happened at the same time and in the same 
> repo.
> +New releases contained the latest OVS and OVN changes. But now these 
> assumptions
> +are no longer valid, and so care must be taken to maintain compatibility.

"no issue with compatibility" significantly overstates the case, since
a distributed system has to be upgraded in some order (and different
parties actually wanted different orders!).  I agree with the larger
point that the split increases the difficulty.

> +Compile-time Incompatibility
> +
> +
> +The first way that the projects can become incompatible is if the C code for 
> OVN
> +no longer can compile.
> +
> +The most likely case for this would be that an OVN change requires a parallel
> +change to OVS. Those keeping up to date with OVN but not OVS will find that 
> OVN
> +will no longer compile since it refers to a nonexistent function or out of 
> date
> +function in OVS.
> +
> +Most OVN users will consume OVN via package from their distribution of 
> choice.
> +OVN consumes libopenvswitch statically, so even if the version of OVS 
> installed
> +on a user's machine is incompatible at compile time, it will not matter.
> +
> +OVN developers are the only ones that would be inconvenienced by a 
> compile-time
> +incompatibility. OVN developers will be expected to regularly update the 
> version
> +of OVS they are using. If an OVN developer notices that OVN is not compiling,
> +then they should update their OVS code to the latest and try again.
> +
> +Developers who are making changes to both OVS and OVN at the same time *must*
> +contribute the OVS change first and ensure it is merged upstream before
> +submitting the OVN change. This way, OVN should never be in a state where it
> +will not compile.
> +
> +When compiling older releases of OVN, it should be able to compile against 
> newer
> +versions of OVS due to API and ABI guarantees in OVS's libaries.

The above section doesn't observe the difference between minor and
major newer versions.  I believe that our current ABI/API guarantee is
for different minro versions within a major release.  It is not
necessarily the case that a newer major release has compatible ABI/API
with an older one.  (I think that we use library naming to make this
clear.)

> +Runtime Incompatibility
> +---
> +
> +The next way that the projects may become incompatible is at runtime. The 
> most
> +common way this would happen is if new OpenFlow capabilities are added to 
> OVS as
> +part of an OVN change. In this case, if someone updates OVN but does not also
> +updage OVS, then OVN will not be able to install the OpenFlow rules it wishes
> +to.
> +
> +Unlike with compile-time incompatibilities, we can't wallpaper over the fact
> +that the OVS installation is not up to date. The best we can do is make it 
> very
> +clear at runtime that a certain feature is not present, and if the feature is
> +desired, OVS must be upgraded.
> +
> +The following is the process that OVN developers should use when making a
> +runtime compatibility change to OVS and OVN.
> +
> +1. Submit the change to OVS first. See the change through until it is merged.
> +2. Make the necessary changes to OVN. 
> +
> +  a. At startup, probe OVS for the existence of the OpenFlow addition. If it
> + is not present, then output an informational message that explains which
> + OVN feature(s) cannot be used.
> +  b. If a user attempts to explicitly configure the feature that is not 
> usable
> + due to the incompatibility, then output a warning message.
> +  c. Ensure that the code that installs the OpenFlow will only do so if the 
> new
> + feature is present.

It's possible that we could introduce features for logical flows that
allow different behavior based on whether a given node supports a
particular feature.  I've thought about this in the past, but it
didn't become necessary yet.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] Querying OVS for OpenFlow capabilities

[Mark Michelson]

On 9/12/19 2:46 PM, Justin Pettit wrote:




On Sep 12, 2019, at 11:08 AM, Mark Michelson  wrote:

Hi guys,

I recently sent a new version of a document describing proposed OVN/OVS 
compatibility [1]. In it, I suggested that when we add new OpenFlow 
capabilities to OVS, then OVN needs to probe OVS for the OpenFlow capabilities 
at startup. This way we can know whether we have the capability, can log an 
informational message, and can just generally be as graceful as possible.

Numan brought up that the ability to probe at runtime for an OpenFlow feature 
may not exist yet. I'm aware right now that we could attempt to program a flow 
that uses the capability we're interested in and see if it succeeds. But I was 
wondering if there's a more formal way of doing it.


Thanks for writing up that document.  I do think probing is the best way to do 
it.  We do similar feature probing of the datapath in ovs-vswitchd (see 
dpif_probe_feature() in lib/dpif.c).

This brings up another wrinkle: datapath compatibility.  Feature support 
changes depending on the datapath, and there's starting to be a proliferation 
of datapaths (upstream Linux kernel datapath, OVS Linux kernel datapath, 
Windows datapath, and userspace datapath).  These have different features that 
can affect the ability to make OVS work as the controller expects.

William (cc'd) is looking at adding a new feature compatibility column to the 
Datapath table that should be landing in master this week.  This will allow a 
controller to probe for datapath-level features.  (And some of those could 
probably be proxies for OpenFlow-level support for features.)  I'm not sure if 
he's started coding it yet, though.


I think this could cover us. The big thing we want to be able to do is 
to probe for the existence of a runtime feature. If the datapath 
determines the existence of that feature, then being able to query the 
datapath for the feature makes sense.




Anyway, something else you may want to keep an eye on.

--Justin




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

Re: [ovs-dev] Querying OVS for OpenFlow capabilities

[Justin Pettit]

> On Sep 12, 2019, at 11:08 AM, Mark Michelson  wrote:
> 
> Hi guys,
> 
> I recently sent a new version of a document describing proposed OVN/OVS 
> compatibility [1]. In it, I suggested that when we add new OpenFlow 
> capabilities to OVS, then OVN needs to probe OVS for the OpenFlow 
> capabilities at startup. This way we can know whether we have the capability, 
> can log an informational message, and can just generally be as graceful as 
> possible.
> 
> Numan brought up that the ability to probe at runtime for an OpenFlow feature 
> may not exist yet. I'm aware right now that we could attempt to program a 
> flow that uses the capability we're interested in and see if it succeeds. But 
> I was wondering if there's a more formal way of doing it.

Thanks for writing up that document.  I do think probing is the best way to do 
it.  We do similar feature probing of the datapath in ovs-vswitchd (see 
dpif_probe_feature() in lib/dpif.c).

This brings up another wrinkle: datapath compatibility.  Feature support 
changes depending on the datapath, and there's starting to be a proliferation 
of datapaths (upstream Linux kernel datapath, OVS Linux kernel datapath, 
Windows datapath, and userspace datapath).  These have different features that 
can affect the ability to make OVS work as the controller expects.

William (cc'd) is looking at adding a new feature compatibility column to the 
Datapath table that should be landing in master this week.  This will allow a 
controller to probe for datapath-level features.  (And some of those could 
probably be proxies for OpenFlow-level support for features.)  I'm not sure if 
he's started coding it yet, though.

Anyway, something else you may want to keep an eye on.

--Justin


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

[ovs-dev] [PATCH] checkpatch: Ignore utitilies/bugtool.

[William Tu]

Signed-off-by: William Tu 
---
 utilities/checkpatch.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index f8fa00e306a8..a9f27b52f3c8 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -844,6 +844,8 @@ def ovs_checkpatch_parse(text, filename, author=None, 
committer=None):
 # for a common style.
 if current_file.startswith('include/sparse'):
 continue
+if current_file.startswith('utilities/bugtool'):
+continue
 run_checks(current_file, cmp_line, lineno)
 
 run_file_checks(text)
-- 
2.7.4

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

Re: [ovs-dev] [PATCH] ovs-bugtool: Add ip -s -s to get_device_stats.out.

[William Tu]

On Thu, Sep 12, 2019 at 11:01 AM 0-day Robot  wrote:
>
> Bleep bloop.  Greetings William Tu, I am a robot and I have tried out your 
> patch.
> Thanks for your contribution.
>
> I encountered some error that I wasn't expecting.  See the details below.
>
>
> checkpatch:
> WARNING: Line is 80 characters long (recommended limit is 79)
> #24 FILE: utilities/bugtool/plugins/network-status/openvswitch.xml:43:
> ip -s -s link 
> show
>
This is xml file and I think it's OK for more than 79 char.
I will submit a patch to skip checking for utilities/bugtool/

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

[ovs-dev] Querying OVS for OpenFlow capabilities

[Mark Michelson]

Hi guys,

I recently sent a new version of a document describing proposed OVN/OVS 
compatibility [1]. In it, I suggested that when we add new OpenFlow 
capabilities to OVS, then OVN needs to probe OVS for the OpenFlow 
capabilities at startup. This way we can know whether we have the 
capability, can log an informational message, and can just generally be 
as graceful as possible.


Numan brought up that the ability to probe at runtime for an OpenFlow 
feature may not exist yet. I'm aware right now that we could attempt to 
program a flow that uses the capability we're interested in and see if 
it succeeds. But I was wondering if there's a more formal way of doing it.


Thanks,
Mark Michelson

[1] http://patchwork.ozlabs.org/patch/1159217/
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovs-bugtool: Add ip -s -s to get_device_stats.out.

[0-day Robot]

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

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


checkpatch:
WARNING: Line is 80 characters long (recommended limit is 79)
#24 FILE: utilities/bugtool/plugins/network-status/openvswitch.xml:43:
ip -s -s link show

Lines checked: 28, Warnings: 1, Errors: 0


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

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

Re: [ovs-dev] [PATCH 1/1] stream_ssl: fix important memory leak in ssl_connect() function

[William Tu]

On Fri, Jul 26, 2019 at 10:11:03AM +0200, Damijan Skvarc wrote:
> 
> While checking valgrind reports after running "make check-valgrind" I have 
> noticed 
> reports for several tests similar to the following:
> 
> 
> ==5345== Memcheck, a memory error detector
> ==5345== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
> ==5345== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
> ==5345== Command: ovsdb-client 
> --private-key=/home/damijan.skvarc/doma/ovs/tests/testpki-privkey.pem 
> --certificate=/home/damijan.skvarc/doma/ovs/tests/testpki-cert.pem 
> --ca-cert=/home/damijan.skvarc/doma/ovs/tests/testpki-cacert.pem transact 
> ssl:127.0.0.1:40111 \ \ \ ["ordinals",
> ==5345== \ \ \ \ \ \ {"op":\ "update",
> ==5345== \ \ \ \ \ \ \ "table":\ "ordinals",
> ==5345== \ \ \ \ \ \ \ "where":\ [["number",\ "==",\ 1]],
> ==5345== \ \ \ \ \ \ \ "row":\ {"number":\ 2,\ "name":\ "old\ two"}},
> ==5345== \ \ \ \ \ \ {"op":\ "update",
> ==5345== \ \ \ \ \ \ \ "table":\ "ordinals",
> ==5345== \ \ \ \ \ \ \ "where":\ [["name",\ "==",\ "two"]],
> ==5345== \ \ \ \ \ \ \ "row":\ {"number":\ 1,\ "name":\ "old\ one"}}]
> ==5345== Parent PID: 5344
> ==5345== 
> ==5345== 
> ==5345== HEAP SUMMARY:
> ==5345== in use at exit: 116,551 bytes in 3,341 blocks
> ==5345==   total heap usage: 5,134 allocs, 1,793 frees, 412,290 bytes 
> allocated
> ==5345== 
> ==5345== 6,221 (184 direct, 6,037 indirect) bytes in 1 blocks are definitely 
> lost in loss record 498 of 500
> ==5345==at 0x4C2DB8F: malloc (in 
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==5345==by 0x5105E77: CRYPTO_malloc (in 
> /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
> ==5345==by 0x51E1D23: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
> ==5345==by 0x51E4861: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
> ==5345==by 0x51E5414: ASN1_item_ex_d2i (in 
> /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
> ==5345==by 0x51E546A: ASN1_item_d2i (in 
> /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
> ==5345==by 0x4E56B27: ??? (in /lib/x86_64-linux-gnu/libssl.so.1.0.0)
> ==5345==by 0x4E5BA11: ??? (in /lib/x86_64-linux-gnu/libssl.so.1.0.0)
> ==5345==by 0x4E65145: ??? (in /lib/x86_64-linux-gnu/libssl.so.1.0.0)
> ==5345==by 0x4522DF: ssl_connect (stream-ssl.c:530)
> ==5345==by 0x443D38: scs_connecting (stream.c:315)
> ==5345==by 0x443D38: stream_connect (stream.c:338)
> ==5345==by 0x443FA1: stream_open_block (stream.c:266)
> ==5345==by 0x40AB79: open_jsonrpc (ovsdb-client.c:507)
> ==5345==by 0x40AB79: open_rpc (ovsdb-client.c:143)
> ==5345==by 0x40B06B: do_transact__ (ovsdb-client.c:871)
> ==5345==by 0x40B245: do_transact (ovsdb-client.c:893)
> ==5345==by 0x405F76: main (ovsdb-client.c:282)
> ==5345== 
> ==5345== LEAK SUMMARY:
> ==5345==definitely lost: 184 bytes in 1 blocks
> ==5345==indirectly lost: 6,037 bytes in 117 blocks
> ==5345==  possibly lost: 0 bytes in 0 blocks
> ==5345==still reachable: 110,330 bytes in 3,223 blocks
> ==5345== suppressed: 0 bytes in 0 blocks
> ==5345== Reachable blocks (those to which a pointer was found) are not shown.
> ==5345== To see them, rerun with: --leak-check=full --show-leak-kinds=all
> ==5345== 
> ==5345== For counts of detected and suppressed errors, rerun with: -v
> ==5345== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
> 
> 
> 
> This report was extracted from "index uniqueness checking" test and complains 
> about 
> leaking memory in ovsdb-client application. The problem is not huge, since 
> ovsdb-client 
> is CLI tool which is constantly reinvoked/restarted, thus leaked memory is 
> not accumulated.
> 
> More problematic issue is that for the same test valgrind reports the similar 
> problem also for 
> ovsdb-server:
> 
> 
> ==5290== Memcheck, a memory error detector
> ==5290== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
> ==5290== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
> ==5290== Command: ovsdb-server --log-file --detach --no-chdir --pidfile 
> --private-key=/home/damijan.skvarc/doma/ovs/tests/testpki-privkey2.pem 
> --certificate=/home/damijan.skvarc/doma/ovs/tests/testpki-cert2.pem 
> --ca-cert=/home/damijan.skvarc/doma/ovs/tests/testpki-cacert.pem 
> --remote=pssl:0:127.0.0.1 db
> ==5290== Parent PID: 5289
> ==5290== 
> ==5292== Warning: noted but unhandled ioctl 0x2403 with no size/direction 
> hints.
> ==5292==This could cause spurious value errors to appear.
> ==5292==See README_MISSING_SYSCALL_OR_IOCTL for guidance on writing a 
> proper wrapper.
> ==5292== Warning: noted but unhandled ioctl 0x2400 with no size/direction 
> hints.
> ==5292==This could cause spurious value errors to appear.
> ==5292==See README_MISSING_SYSCALL_OR_IOCTL for guidance on writing a 
> proper wrapper.
> ==5290== 
> ==5290== HEAP SUMMARY:
> ==5290== in use at exit: 2,066 bytes in 48 blocks
> ==5290==   total heap usage: 

Re: [ovs-dev] [PATCH] vswitch: ratelimit the device add log

[William Tu]

On Thu, Sep 12, 2019 at 11:45:41AM -0400, Aaron Conole wrote:
> It's possible that a port added to the system with certain kinds
> of invalid parameters will cause the 'could not add' log to be
> triggered.  When this happens, the vswitch run loop can continually
> re-attempt adding the port.  While the parameters remain invalid
> the vswitch run loop will re-trigger the warning, flooding the
> syslog.
> 
> This patch adds a simple rate limit to the log.
> 
> Signed-off-by: Aaron Conole 

LGTM, hit this issue quite often.

Acked-by: William Tu 

> ---
>  vswitchd/bridge.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index d921c4ef8..49a6f6a37 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -1816,8 +1816,12 @@ iface_do_create(const struct bridge *br,
>  *ofp_portp = iface_pick_ofport(iface_cfg);
>  error = ofproto_port_add(br->ofproto, netdev, ofp_portp);
>  if (error) {
> -VLOG_WARN_BUF(errp, "could not add network device %s to ofproto 
> (%s)",
> -  iface_cfg->name, ovs_strerror(error));
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> +if (!VLOG_DROP_WARN()) {
> +VLOG_WARN_BUF(errp,
> +  "could not add network device %s to ofproto (%s)",
> +  iface_cfg->name, ovs_strerror(error));
> +}
>  goto error;
>  }
>  
> -- 
> 2.21.0
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] ovs-bugtool: Add ip -s -s to get_device_stats.out.

[William Tu]

The patch adds 'ip -s -s' to file get_device_stats.out to collect
device statistics. When debugging tunnel related issues, the command
shows much more detailed counters, ex: frame, crc, carrier, helping
to understand the root cause when packets are dropped.

Signed-off-by: William Tu 
---
 utilities/bugtool/plugins/network-status/openvswitch.xml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/utilities/bugtool/plugins/network-status/openvswitch.xml 
b/utilities/bugtool/plugins/network-status/openvswitch.xml
index d39867c6e4d0..b0e7a15103b0 100644
--- a/utilities/bugtool/plugins/network-status/openvswitch.xml
+++ b/utilities/bugtool/plugins/network-status/openvswitch.xml
@@ -40,4 +40,5 @@
 /usr/share/openvswitch/scripts/ovs-bugtool-ovs-ofctl-loop-over-bridges
 "dump-groups"
 /usr/share/openvswitch/scripts/ovs-bugtool-ovs-ofctl-loop-over-bridges
 "dump-group-stats"
 /usr/share/openvswitch/scripts/ovs-bugtool-get-dpdk-nic-numa
+ip -s -s link 
show
 
-- 
2.7.4

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

[ovs-dev] [PATCH] dpdk: trivial: Fix comment and cast.

[William Tu]

Comment should start with capital letter.
Remove unnecessary type casting.

Signed-off-by: William Tu 
---
 lib/dp-packet.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index 14f0897fa637..5f703ad457b3 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -40,7 +40,7 @@ enum OVS_PACKED_ENUM dp_packet_source {
 DPBUF_MALLOC,  /* Obtained via malloc(). */
 DPBUF_STACK,   /* Un-movable stack space or static buffer. */
 DPBUF_STUB,/* Starts on stack, may expand into heap. */
-DPBUF_DPDK,/* buffer data is from DPDK allocated memory.
+DPBUF_DPDK,/* Buffer data is from DPDK allocated memory.
 * ref to dp_packet_init_dpdk() in dp-packet.c.
 */
 DPBUF_AFXDP,   /* Buffer data from XDP frame. */
@@ -191,7 +191,7 @@ dp_packet_delete(struct dp_packet *b)
 if (b->source == DPBUF_DPDK) {
 /* If this dp_packet was allocated by DPDK it must have been
  * created as a dp_packet */
-free_dpdk_buf((struct dp_packet*) b);
+free_dpdk_buf(b);
 return;
 }
 
-- 
2.7.4

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

Re: [ovs-dev] Virtio Crypto with OVS-DPDK

[Ilya Maximets]

On 12.09.2019 18:35, Harish Kumar Ambati wrote:
> Thanks  IIya for the reply . Please find my comment below.
> 
>>>Hi Harish,
> 
>>>Why do you want to integrate virtio-crypto into OVS?
>>>OVS is an OpenFlow Network Switch, but crypto devices are not network 
>>>devices.
>>>They will not fit in OVS purposes nor OVS architecture.
> 
> Let's take a use-case of ipsec running in a guest. A guest with a virtio_net 
> driver, shares a number of virtqueues with QEMU.
> Virtio-net backend is supported by OVS. Now, if we want to use crypto device 
> with virtio-crypto? How should we do it without OVS? 
> I felt OVS is a right place to also provide support for virtio-crypto - 
> especially when it is being used in conjunction with virtio-net.

virtio-crypto is a separate device with its own virtqueues.
It doesn't have any connection to virtio-net device.
If application in guest will want to send ciphered traffic, it
will first send the data to virtio-crypto device to be encrypted,
will receive it back from the crypto device and only after that it
will send resulted encrypted packet to virtio-net, where it will
be received by OVS and sent to the wild network.

So, any other application could be a backend for virtio-crypto
device. You could run same crypto example from DPDK along with
OVS. OVS will be responsible for networking and cprypto example
will be responsible for encrypting.

> 
> Please suggest.
> 
>  
> 
> Regards,
> 
> Harish
> 
> 
> -Original Message-
> From: ovs-dev-boun...@openvswitch.org  On 
> Behalf Of Ilya Maximets
> Sent: Tuesday, September 10, 2019 2:02 PM
> To: ovs-dev@openvswitch.org
> Subject: Re: [ovs-dev] Virtio Crypto with OVS-DPDK
> 
>  
> 
>> Hi All,
> 
>>
> 
>> I am working on Virtio Crypto and could run DPDK virtio crypto use cases on 
>> both x86 and ARM.
> 
>>
> 
>> Now ,  I am trying to integrate virtio crypto with ovs-dpdk , is there any 
>> work done in this direction . Could someone please help with inputs.
> 
>  
> 
>>>Hi Harish,
> 
>>>Why do you want to integrate virtio-crypto into OVS?
> 
>>>OVS is an OpenFlow Network Switch, but crypto devices are not network 
>>>devices.
> 
>>>They will not fit in OVS purposes nor OVS architecture.
> 
>  
> 
> Let's take a use-case of ipsec running in a guest. A guest with a virtio_net 
> driver, shares a number of virtqueues with QEMU.
> 
> Virtio-net backend is supported by OVS. Now, if we want to use crypto device 
> with virtio-crypto? How should we do it without OVS? 
> 
> I felt OVS is a right place to also provide support for virtio-crypto - 
> specially when it is being used in conjunction with virtio-net.Please suggest.
> 
>  
> 
>  
> 
> Best regards, Ilya Maximets.
> 
>  
> 
>>
> 
>> Below is the DPDK sample application we tried.
> 
>>
> 
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdoc.dpdk.org%2Fguides%2Fsample_app_ug%2Fvhost_crypto.htmldata=02%7C01%7Charish.ambati%40nxp.com%7C71f7b0c627d94851911408d735c95903%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637037011215155264sdata=iVd%2FR747iUd%2BlxwFXNPP%2BUFdClGoat8DTEYkX9ZpCa4%3Dreserved=0
> 
>>
> 
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fspp-tmp.readthedocs.io%2Fen%2Fstable%2Fcryptodevs%2Fvirtio.htmldata=02%7C01%7Charish.ambati%40nxp.com%7C71f7b0c627d94851911408d735c95903%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637037011215155264sdata=OoPgNStH42qfPd4tG7H0JkeLOp5jR5Pz7G2aEZsNxZ0%3Dreserved=0
> 
>>
> 
>>
> 
>> Regards,
> 
>> Harish
> 
>> Software R
> 
>> Hyderabad Design Center (HDC), NXP India
> 
>> Mob:9885996745 work: 91-4033504056
> 
> ___
> 
> dev mailing list
> 
> d...@openvswitch.org 
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-devdata=02%7C01%7Charish.ambati%40nxp.com%7C71f7b0c627d94851911408d735c95903%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637037011215155264sdata=gdI1%2FMhMj756sf7gvA2i3vAMCnAE2NxRI5Qki1QAOjo%3Dreserved=0
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] vswitch: ratelimit the device add log

[Aaron Conole]

It's possible that a port added to the system with certain kinds
of invalid parameters will cause the 'could not add' log to be
triggered.  When this happens, the vswitch run loop can continually
re-attempt adding the port.  While the parameters remain invalid
the vswitch run loop will re-trigger the warning, flooding the
syslog.

This patch adds a simple rate limit to the log.

Signed-off-by: Aaron Conole 
---
 vswitchd/bridge.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index d921c4ef8..49a6f6a37 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -1816,8 +1816,12 @@ iface_do_create(const struct bridge *br,
 *ofp_portp = iface_pick_ofport(iface_cfg);
 error = ofproto_port_add(br->ofproto, netdev, ofp_portp);
 if (error) {
-VLOG_WARN_BUF(errp, "could not add network device %s to ofproto (%s)",
-  iface_cfg->name, ovs_strerror(error));
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+if (!VLOG_DROP_WARN()) {
+VLOG_WARN_BUF(errp,
+  "could not add network device %s to ofproto (%s)",
+  iface_cfg->name, ovs_strerror(error));
+}
 goto error;
 }
 
-- 
2.21.0

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

Re: [ovs-dev] Virtio Crypto with OVS-DPDK

[Harish Kumar Ambati]

Thanks  IIya for the reply . Please find my comment below.



>>Hi Harish,

>>Why do you want to integrate virtio-crypto into OVS?

>>OVS is an OpenFlow Network Switch, but crypto devices are not network devices.

>>They will not fit in OVS purposes nor OVS architecture.



Let's take a use-case of ipsec running in a guest. A guest with a virtio_net 
driver, shares a number of virtqueues with QEMU.

Virtio-net backend is supported by OVS. Now, if we want to use crypto device 
with virtio-crypto? How should we do it without OVS?

I felt OVS is a right place to also provide support for virtio-crypto - 
especially when it is being used in conjunction with virtio-net.

Please suggest.



Regards,

Harish





-Original Message-
From: ovs-dev-boun...@openvswitch.org  On 
Behalf Of Ilya Maximets
Sent: Tuesday, September 10, 2019 2:02 PM
To: ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] Virtio Crypto with OVS-DPDK



> Hi All,

>

> I am working on Virtio Crypto and could run DPDK virtio crypto use cases on 
> both x86 and ARM.

>

> Now ,  I am trying to integrate virtio crypto with ovs-dpdk , is there any 
> work done in this direction . Could someone please help with inputs.



>>Hi Harish,

>>Why do you want to integrate virtio-crypto into OVS?

>>OVS is an OpenFlow Network Switch, but crypto devices are not network devices.

>>They will not fit in OVS purposes nor OVS architecture.



Let's take a use-case of ipsec running in a guest. A guest with a virtio_net 
driver, shares a number of virtqueues with QEMU.

Virtio-net backend is supported by OVS. Now, if we want to use crypto device 
with virtio-crypto? How should we do it without OVS?

I felt OVS is a right place to also provide support for virtio-crypto - 
specially when it is being used in conjunction with virtio-net.Please suggest.





Best regards, Ilya Maximets.



>

> Below is the DPDK sample application we tried.

>

> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdoc.dpdk.org%2Fguides%2Fsample_app_ug%2Fvhost_crypto.htmldata=02%7C01%7Charish.ambati%40nxp.com%7C71f7b0c627d94851911408d735c95903%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637037011215155264sdata=iVd%2FR747iUd%2BlxwFXNPP%2BUFdClGoat8DTEYkX9ZpCa4%3Dreserved=0

>

> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fspp-tmp.readthedocs.io%2Fen%2Fstable%2Fcryptodevs%2Fvirtio.htmldata=02%7C01%7Charish.ambati%40nxp.com%7C71f7b0c627d94851911408d735c95903%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637037011215155264sdata=OoPgNStH42qfPd4tG7H0JkeLOp5jR5Pz7G2aEZsNxZ0%3Dreserved=0

>

>

> Regards,

> Harish

> Software R

> Hyderabad Design Center (HDC), NXP India

> Mob:9885996745 work: 91-4033504056

___

dev mailing list

d...@openvswitch.org

https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-devdata=02%7C01%7Charish.ambati%40nxp.com%7C71f7b0c627d94851911408d735c95903%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637037011215155264sdata=gdI1%2FMhMj756sf7gvA2i3vAMCnAE2NxRI5Qki1QAOjo%3Dreserved=0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3 ovn 0/2] ovn-controller: Logical flow processing optimizations

[Mark Michelson]

Acked-by: Mark Michelson 

On 9/12/19 10:13 AM, Dumitru Ceara wrote:

This series adds some (independent) optimizations that improve
ovn-controller performance by lowering the number of operations that
need to be executed during a iteration of the main controller loop.

Each patch in the series addresses a bottleneck reported by running
`perf record -g --call-graph dwarf -i /usr/bin/ovn-controller` on a
system where the Southbound DB was already populated by ovn-northd.

The logical configuration of the OVN is:
- 500 logical routers
- 1 logical switch attached to each router
- 2 logical ports attached to each switch
- 4 ACLs per switch

To evaluate the performance impact of the change in a more realistic
way the following test is performed (on a single node that runs ovn-northd
and ovn-controller and emulates the VM hosts with OVS internal interfaces):
1. Start ovs-vswitchd.
2. Remove the Southbound DB such that ovn-northd needs to repopulate
everything based on the Northbound DB contents.
3. Start a patched version of ovn-controller such that it tracks loop time
execution information and how long it takes to process all logical flows
from the Southbound DB.
4. At the same time with step 3 above, start ovn-northd which will populate
the Southbound DB.

The following measurements are taken:
- real time (measured with `time`) to have all corresponding openflow flows
   processed by ovs-vswitchd.
- maximum ovn-controller main loop iteration duration
- average ovn-controller main loop iteration duration
- number of times ovn-controller executes the main loop until all logical
   flows are processed

Comparing the results before and after this series we see:
- 13% performance boost w.r.t. real time taken to have all openflow flows
   installed in ovs-vswitchd
- 41% decrease of average ovn-controller loop iteration duration and
   12% increase of ovn-controller number of loop iterations. Essentially
   processing loops are faster and can happen more often.
- 2% decrease of maximum ovn-controller main loop iteration duration.


Dumitru Ceara (2):
   ovn-controller: Optimize update of ct-zones external-ids.
   ovn-controller: Minimize SB DB port_binding lookups.


  controller/binding.c|   19 ---
  controller/ovn-controller.c |   53 +--
  controller/ovn-controller.h |   11 -
  controller/physical.c   |   17 --
  controller/pinctrl.c|   53 +++
  5 files changed, 93 insertions(+), 60 deletions(-)


---
v3:
   - Remove first patch from the series as it was already applied.
   - Rebase rest of series.
v2: Send series for OVN repo instead of OVS.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev



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

Re: [ovs-dev] [PATCH v2 ovn 3/3] ovn-controller: Minimize SB DB port_binding lookups.

[Dumitru Ceara]

On Wed, Aug 28, 2019 at 2:36 PM Numan Siddique  wrote:
>
>
>
> On Fri, Aug 23, 2019 at 4:55 PM Dumitru Ceara  wrote:
>>
>> Instead of storing only peer_ports in struct local_datapath, store both
>> local-remote mappings for patch ports. Also, it's useful to directly
>> store sbrec_port_binding pointers for all datapath ports as we avoid
>> doing costly sbrec_port_binding index lookups by port name.
>>
>> Signed-off-by: Dumitru Ceara 
>
>
> This patch needs a rebase.
>
> Thanks
> Numan

Done, I sent v3:
https://patchwork.ozlabs.org/project/openvswitch/list/?series=130413

Thanks,
Dumitru

>
>>
>> ---
>>  controller/binding.c|   19 ---
>>  controller/ovn-controller.h |   11 -
>>  controller/physical.c   |4 ++-
>>  controller/pinctrl.c|   53 
>> +++
>>  4 files changed, 41 insertions(+), 46 deletions(-)
>>
>> diff --git a/controller/binding.c b/controller/binding.c
>> index 47d4fea..242163d 100644
>> --- a/controller/binding.c
>> +++ b/controller/binding.c
>> @@ -160,13 +160,24 @@ add_local_datapath__(struct ovsdb_idl_index 
>> *sbrec_datapath_binding_by_key,
>>   peer->datapath, false,
>>   depth + 1, local_datapaths);
>>  ld->n_peer_ports++;
>> -ld->peer_ports = xrealloc(ld->peer_ports,
>> -  ld->n_peer_ports *
>> -  sizeof *ld->peer_ports);
>> -ld->peer_ports[ld->n_peer_ports - 1] = peer;
>> +if (ld->n_peer_ports > ld->n_allocated_peer_ports) {
>> +ld->peer_ports =
>> +x2nrealloc(ld->peer_ports,
>> +   >n_allocated_peer_ports,
>> +   sizeof *ld->peer_ports);
>> +}
>> +ld->peer_ports[ld->n_peer_ports - 1].local = pb;
>> +ld->peer_ports[ld->n_peer_ports - 1].remote = peer;
>>  }
>>  }
>>  }
>> +
>> +ld->n_ports++;
>> +if (ld->n_ports > ld->n_allocated_ports) {
>> +ld->ports = x2nrealloc(ld->ports, >n_allocated_ports,
>> +   sizeof *ld->ports);
>> +}
>> +ld->ports[ld->n_ports - 1] = pb;
>>  }
>>  sbrec_port_binding_index_destroy_row(target);
>>  }
>> diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h
>> index 41feec3..86b300e 100644
>> --- a/controller/ovn-controller.h
>> +++ b/controller/ovn-controller.h
>> @@ -60,8 +60,17 @@ struct local_datapath {
>>   * hypervisor. */
>>  bool has_local_l3gateway;
>>
>> -const struct sbrec_port_binding **peer_ports;
>> +const struct sbrec_port_binding **ports;
>> +size_t n_ports;
>> +size_t n_allocated_ports;
>> +
>> +struct {
>> +const struct sbrec_port_binding *local;
>> +const struct sbrec_port_binding *remote;
>> +} *peer_ports;
>> +
>>  size_t n_peer_ports;
>> +size_t n_allocated_peer_ports;
>>  };
>>
>>  struct local_datapath *get_local_datapath(const struct hmap *,
>> diff --git a/controller/physical.c b/controller/physical.c
>> index a05962b..49bc1a4 100644
>> --- a/controller/physical.c
>> +++ b/controller/physical.c
>> @@ -266,11 +266,13 @@ put_replace_router_port_mac_flows(const struct
>>  }
>>
>>  for (int i = 0; i < ld->n_peer_ports; i++) {
>> -const struct sbrec_port_binding *rport_binding = ld->peer_ports[i];
>> +const struct sbrec_port_binding *rport_binding;
>>  struct eth_addr router_port_mac;
>>  struct match match;
>>  struct ofpact_mac *replace_mac;
>>
>> +rport_binding = ld->peer_ports[i].remote;
>> +
>>  /* Table 65, priority 150.
>>   * ===
>>   *
>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
>> index 21ed75f..a8ff0bb 100644
>> --- a/controller/pinctrl.c
>> +++ b/controller/pinctrl.c
>> @@ -174,8 +174,7 @@ static struct pinctrl pinctrl;
>>  static void init_buffered_packets_map(void);
>>  static void destroy_buffered_packets_map(void);
>>  static void
>> -run_buffered_binding(struct ovsdb_idl_index *sbrec_port_binding_by_datapath,
>> - struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
>> +run_buffered_binding(struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
>>   const struct hmap *local_datapaths)
>>  OVS_REQUIRES(pinctrl_mutex);
>>
>> @@ -239,10 +238,7 @@ static void wait_controller_event(struct ovsdb_idl_txn 
>> *ovnsb_idl_txn);
>>  static void init_ipv6_ras(void);
>>  static void destroy_ipv6_ras(void);
>>  static void ipv6_ra_wait(long long int send_ipv6_ra_time);
>> -static void prepare_ipv6_ras(
>> -struct ovsdb_idl_index *sbrec_port_binding_by_datapath,
>> -struct 

[ovs-dev] [PATCH v3 ovn 2/2] ovn-controller: Minimize SB DB port_binding lookups.

[Dumitru Ceara]

Instead of storing only peer_ports in struct local_datapath, store both
local-remote mappings for patch ports. Also, it's useful to directly
store sbrec_port_binding pointers for all datapath ports as we avoid
doing costly sbrec_port_binding index lookups by port name.

Signed-off-by: Dumitru Ceara 
---
 controller/binding.c|   19 ---
 controller/ovn-controller.h |   11 -
 controller/physical.c   |   17 --
 controller/pinctrl.c|   53 +++
 4 files changed, 48 insertions(+), 52 deletions(-)

diff --git a/controller/binding.c b/controller/binding.c
index 47d4fea..242163d 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -160,13 +160,24 @@ add_local_datapath__(struct ovsdb_idl_index 
*sbrec_datapath_binding_by_key,
  peer->datapath, false,
  depth + 1, local_datapaths);
 ld->n_peer_ports++;
-ld->peer_ports = xrealloc(ld->peer_ports,
-  ld->n_peer_ports *
-  sizeof *ld->peer_ports);
-ld->peer_ports[ld->n_peer_ports - 1] = peer;
+if (ld->n_peer_ports > ld->n_allocated_peer_ports) {
+ld->peer_ports =
+x2nrealloc(ld->peer_ports,
+   >n_allocated_peer_ports,
+   sizeof *ld->peer_ports);
+}
+ld->peer_ports[ld->n_peer_ports - 1].local = pb;
+ld->peer_ports[ld->n_peer_ports - 1].remote = peer;
 }
 }
 }
+
+ld->n_ports++;
+if (ld->n_ports > ld->n_allocated_ports) {
+ld->ports = x2nrealloc(ld->ports, >n_allocated_ports,
+   sizeof *ld->ports);
+}
+ld->ports[ld->n_ports - 1] = pb;
 }
 sbrec_port_binding_index_destroy_row(target);
 }
diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h
index 41feec3..86b300e 100644
--- a/controller/ovn-controller.h
+++ b/controller/ovn-controller.h
@@ -60,8 +60,17 @@ struct local_datapath {
  * hypervisor. */
 bool has_local_l3gateway;
 
-const struct sbrec_port_binding **peer_ports;
+const struct sbrec_port_binding **ports;
+size_t n_ports;
+size_t n_allocated_ports;
+
+struct {
+const struct sbrec_port_binding *local;
+const struct sbrec_port_binding *remote;
+} *peer_ports;
+
 size_t n_peer_ports;
+size_t n_allocated_peer_ports;
 };
 
 struct local_datapath *get_local_datapath(const struct hmap *,
diff --git a/controller/physical.c b/controller/physical.c
index f460867..6e606d3 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -259,11 +259,12 @@ put_remote_port_redirect_bridged(const struct
 
 uint32_t ls_dp_key = 0;
 for (int i = 0; i < ld->n_peer_ports; i++) {
-const struct sbrec_port_binding *sport_binding = ld->peer_ports[i];
-const char *sport_peer_name = smap_get(_binding->options,
-   "peer");
-const char *distributed_port = smap_get(>options,
-"distributed-port");
+const struct sbrec_port_binding *sport_binding =
+ld->peer_ports[i].remote;
+const char *sport_peer_name =
+smap_get(_binding->options, "peer");
+const char *distributed_port =
+smap_get(>options, "distributed-port");
 
 if (!strcmp(sport_peer_name, distributed_port)) {
 ls_dp_key = sport_binding->datapath->tunnel_key;
@@ -525,7 +526,8 @@ put_replace_chassis_mac_flows(const struct simap *ct_zones,
 struct zone_ids zone_ids = get_zone_ids(localnet_port, ct_zones);
 
 for (int i = 0; i < ld->n_peer_ports; i++) {
-const struct sbrec_port_binding *rport_binding = ld->peer_ports[i];
+const struct sbrec_port_binding *rport_binding =
+ld->peer_ports[i].remote;
 struct eth_addr router_port_mac;
 char *err_str = NULL;
 struct match match;
@@ -632,7 +634,8 @@ put_replace_router_port_mac_flows(struct ovsdb_idl_index
 }
 
 for (int i = 0; i < ld->n_peer_ports; i++) {
-const struct sbrec_port_binding *rport_binding = ld->peer_ports[i];
+const struct sbrec_port_binding *rport_binding =
+ld->peer_ports[i].remote;
 struct eth_addr router_port_mac;
 struct match match;
 struct ofpact_mac *replace_mac;
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index cdaf5c1..11cac4f 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -174,8 +174,7 @@ static struct pinctrl pinctrl;
 static void 

[ovs-dev] [PATCH v3 ovn 1/2] ovn-controller: Optimize update of ct-zones external-ids.

[Dumitru Ceara]

commit_ct_zones() is called at every ovn-controller iteration but updates to
ct-zones don't happen at every iteration. Avoid cloning the
br-int->external_ids map unless an update is needed.

Signed-off-by: Dumitru Ceara 
---
 controller/ovn-controller.c |   53 +--
 1 file changed, 45 insertions(+), 8 deletions(-)

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index b5449b8..33ece59 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -531,10 +531,9 @@ static void
 commit_ct_zones(const struct ovsrec_bridge *br_int,
 struct shash *pending_ct_zones)
 {
-struct smap new_ids;
-smap_clone(_ids, _int->external_ids);
+struct smap ct_add_ids = SMAP_INITIALIZER(_add_ids);
+struct sset ct_del_ids = SSET_INITIALIZER(_del_ids);
 
-bool updated = false;
 struct shash_node *iter;
 SHASH_FOR_EACH(iter, pending_ct_zones) {
 struct ct_zone_pending_entry *ctzpe = iter->data;
@@ -550,22 +549,60 @@ commit_ct_zones(const struct ovsrec_bridge *br_int,
 char *user_str = xasprintf("ct-zone-%s", iter->name);
 if (ctzpe->add) {
 char *zone_str = xasprintf("%"PRId32, ctzpe->zone);
-smap_replace(_ids, user_str, zone_str);
+struct smap_node *node =
+smap_get_node(_int->external_ids, user_str);
+if (!node || strcmp(node->value, zone_str)) {
+smap_add_nocopy(_add_ids, user_str, zone_str);
+user_str = NULL;
+zone_str = NULL;
+}
 free(zone_str);
 } else {
-smap_remove(_ids, user_str);
+if (smap_get(_int->external_ids, user_str)) {
+sset_add(_del_ids, user_str);
+}
 }
 free(user_str);
 
 ctzpe->state = CT_ZONE_DB_SENT;
-updated = true;
 }
 
-if (updated) {
+/* Update the bridge external IDs only if really needed (i.e., we must
+ * add a value or delete one). Rebuilding the external IDs map at
+ * every run is a costly operation when having lots of ct_zones.
+ */
+if (!smap_is_empty(_add_ids) || !sset_is_empty(_del_ids)) {
+struct smap new_ids = SMAP_INITIALIZER(_ids);
+
+struct smap_node *id;
+SMAP_FOR_EACH (id, _int->external_ids) {
+if (sset_find_and_delete(_del_ids, id->key)) {
+continue;
+}
+
+if (smap_get(_add_ids, id->key)) {
+continue;
+}
+
+smap_add(_ids, id->key, id->value);
+}
+
+struct smap_node *next_id;
+SMAP_FOR_EACH_SAFE (id, next_id, _add_ids) {
+smap_replace(_ids, id->key, id->value);
+smap_remove_node(_add_ids, id);
+}
+
 ovsrec_bridge_verify_external_ids(br_int);
 ovsrec_bridge_set_external_ids(br_int, _ids);
+smap_destroy(_ids);
 }
-smap_destroy(_ids);
+
+ovs_assert(smap_is_empty(_add_ids));
+ovs_assert(sset_is_empty(_del_ids));
+
+smap_destroy(_add_ids);
+sset_destroy(_del_ids);
 }
 
 static void

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

[ovs-dev] [PATCH v3 ovn 0/2] ovn-controller: Logical flow processing optimizations

[Dumitru Ceara]

This series adds some (independent) optimizations that improve
ovn-controller performance by lowering the number of operations that
need to be executed during a iteration of the main controller loop.

Each patch in the series addresses a bottleneck reported by running
`perf record -g --call-graph dwarf -i /usr/bin/ovn-controller` on a
system where the Southbound DB was already populated by ovn-northd.

The logical configuration of the OVN is:
- 500 logical routers
- 1 logical switch attached to each router
- 2 logical ports attached to each switch
- 4 ACLs per switch

To evaluate the performance impact of the change in a more realistic
way the following test is performed (on a single node that runs ovn-northd
and ovn-controller and emulates the VM hosts with OVS internal interfaces):
1. Start ovs-vswitchd.
2. Remove the Southbound DB such that ovn-northd needs to repopulate
   everything based on the Northbound DB contents.
3. Start a patched version of ovn-controller such that it tracks loop time
   execution information and how long it takes to process all logical flows
   from the Southbound DB.
4. At the same time with step 3 above, start ovn-northd which will populate
   the Southbound DB.

The following measurements are taken:
- real time (measured with `time`) to have all corresponding openflow flows
  processed by ovs-vswitchd.
- maximum ovn-controller main loop iteration duration
- average ovn-controller main loop iteration duration
- number of times ovn-controller executes the main loop until all logical
  flows are processed

Comparing the results before and after this series we see:
- 13% performance boost w.r.t. real time taken to have all openflow flows
  installed in ovs-vswitchd
- 41% decrease of average ovn-controller loop iteration duration and
  12% increase of ovn-controller number of loop iterations. Essentially
  processing loops are faster and can happen more often.
- 2% decrease of maximum ovn-controller main loop iteration duration.


Dumitru Ceara (2):
  ovn-controller: Optimize update of ct-zones external-ids.
  ovn-controller: Minimize SB DB port_binding lookups.


 controller/binding.c|   19 ---
 controller/ovn-controller.c |   53 +--
 controller/ovn-controller.h |   11 -
 controller/physical.c   |   17 --
 controller/pinctrl.c|   53 +++
 5 files changed, 93 insertions(+), 60 deletions(-)


---
v3:
  - Remove first patch from the series as it was already applied.
  - Rebase rest of series.
v2: Send series for OVN repo instead of OVS.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 ovn 2/3] ovn-controller: Optimize update of ct-zones external-ids.

[Dumitru Ceara]

On Wed, Aug 28, 2019 at 2:36 PM Numan Siddique  wrote:
>
>
>
> On Fri, Aug 23, 2019 at 4:54 PM Dumitru Ceara  wrote:
>>
>> commit_ct_zones() is called at every ovn-controller iteration but updates to
>> ct-zones don't happen at every iteration. Avoid cloning the
>> br-int->external_ids map unless an update is needed.
>>
>> Signed-off-by: Dumitru Ceara 
>> ---
>>  controller/ovn-controller.c |   53 
>> +--
>>  1 file changed, 45 insertions(+), 8 deletions(-)
>>
>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> index 86f29ac..1825c1f 100644
>> --- a/controller/ovn-controller.c
>> +++ b/controller/ovn-controller.c
>> @@ -531,10 +531,9 @@ static void
>>  commit_ct_zones(const struct ovsrec_bridge *br_int,
>>  struct shash *pending_ct_zones)
>>  {
>> -struct smap new_ids;
>> -smap_clone(_ids, _int->external_ids);
>> +struct smap ct_add_ids = SMAP_INITIALIZER(_add_ids);
>> +struct sset ct_del_ids = SSET_INITIALIZER(_del_ids);
>>
>> -bool updated = false;
>>  struct shash_node *iter;
>>  SHASH_FOR_EACH(iter, pending_ct_zones) {
>>  struct ct_zone_pending_entry *ctzpe = iter->data;
>> @@ -550,22 +549,60 @@ commit_ct_zones(const struct ovsrec_bridge *br_int,
>>  char *user_str = xasprintf("ct-zone-%s", iter->name);
>>  if (ctzpe->add) {
>>  char *zone_str = xasprintf("%"PRId32, ctzpe->zone);
>> -smap_replace(_ids, user_str, zone_str);
>> +struct smap_node *node =
>> +smap_get_node(_int->external_ids, user_str);
>> +if (!node || strcmp(node->value, zone_str)) {
>> +smap_add_nocopy(_add_ids, user_str, zone_str);
>> +user_str = NULL;
>> +zone_str = NULL;
>> +}
>>  free(zone_str);
>>  } else {
>> -smap_remove(_ids, user_str);
>> +if (smap_get(_int->external_ids, user_str)) {
>> +sset_add(_del_ids, user_str);
>> +}
>>  }
>>  free(user_str);
>>
>>  ctzpe->state = CT_ZONE_DB_SENT;
>> -updated = true;
>>  }
>>
>> -if (updated) {
>> +/* Update the bridge external IDs only if really needed (i.e., we must
>> + * add a value or delete one). Rebuilding the external IDs map at
>> + * every run is a costly operation when having lots of ct_zones.
>> + */
>> +if (!smap_is_empty(_add_ids) || !sset_is_empty(_del_ids)) {
>> +struct smap new_ids = SMAP_INITIALIZER(_ids);
>> +
>> +struct smap_node *id;
>> +SMAP_FOR_EACH (id, _int->external_ids) {
>> +if (sset_find_and_delete(_del_ids, id->key)) {
>> +continue;
>> +}
>> +
>> +if (smap_get(_add_ids, id->key)) {
>
>
> What if the value of this key is changed ?
>
> Suppose we have "a = 1" in external_ids  and the ct_add_ids has "a = 2".
> Looks like in this case, the external_ids column in the db will not be updated
> with "a = 2".
>

Thanks for reviewing this!

If we're adding "a = 2" that means that we'll have the (a, 2) mapping
in ct_add_ids. It was added because strcmp(node->value, zone_str)
returned non-zero above.
This loop builds a copy of external_ids skipping the pairs that need
to be updated or added (ct_add_ids) and those that need to be deleted
(ct_del_ids).
The next loop will do the add/update through smap_replace.

Thanks,
Dumitru

> I am not sure though if this can actually happen.
>
> Thanks
> Numan
>
>> +continue;
>> +}
>> +
>> +smap_add(_ids, id->key, id->value);
>> +}
>> +
>> +struct smap_node *next_id;
>> +SMAP_FOR_EACH_SAFE (id, next_id, _add_ids) {
>> +smap_replace(_ids, id->key, id->value);
>> +smap_remove_node(_add_ids, id);
>> +}
>> +
>>  ovsrec_bridge_verify_external_ids(br_int);
>>  ovsrec_bridge_set_external_ids(br_int, _ids);
>> +smap_destroy(_ids);
>>  }
>> -smap_destroy(_ids);
>> +
>> +ovs_assert(smap_is_empty(_add_ids));
>> +ovs_assert(sset_is_empty(_del_ids));
>> +
>> +smap_destroy(_add_ids);
>> +sset_destroy(_del_ids);
>>  }
>>
>>  static void
>>
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] netdev-dpdk: Fix Tx queue false sharing.

[Eelco Chaudron]

On 26 Aug 2019, at 16:54, Ilya Maximets wrote:

'tx_q' array is allocated for each DPDK netdev.  'struct 
dpdk_tx_queue'

is 8 bytes long, so 8 tx queues are sharing the same cache line in
case of 64B cacheline size.  This causes 'false sharing' issue in
mutliqueue case because taking the spinlock implies write to memory
i.e. cache invalidation.

Signed-off-by: Ilya Maximets 
---

I didn't really test the performance impact yet, so testing is very
welcome.  Relevant test case could be PVP with 8 queues in HW NIC and
8 queues in vhost-user inerface and 8 PMD threads.


Did a quick test on my setup, but I do not have enough cores to do 8 PMD 
threads and 8 VM cores. So I did a 4 core PMD/VM test running PV (not 
PVP as we do not care about receiving traffic from the VHOST) using 64 
bytes packets and 100 L3 flows.


Without the patch, I get 17,408,585 pps and with the patch 17,573,089 
pps.


The patch looks good to me…

Acked-by: Eelco Chaudron 




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

Re: [ovs-dev] [PATCH v8] netdev-dpdk:Detailed packet drop statistics

[Ilya Maximets]

On 10.09.2019 20:31, Sriram Vatala wrote:
> -Original Message-
> From: Ilya Maximets 
> Sent: 10 September 2019 19:29
> To: Sriram Vatala ; ovs-dev@openvswitch.org
> Cc: ktray...@redhat.com; ian.sto...@intel.com
> Subject: Re: [PATCH v8] netdev-dpdk:Detailed packet drop statistics
> 
> On 08.09.2019 19:01, Sriram Vatala wrote:
>> OVS may be unable to transmit packets for multiple reasons and today
>> there is a single counter to track packets dropped due to any of those
>> reasons. The most common reason is that a VM is unable to read packets
>> fast enough causing the vhostuser port transmit queue on the OVS side
>> to become full. This manifests as a problem with VNFs not receiving
>> all packets. Having a separate drop counter to track packets dropped
>> because the transmit queue is full will clearly indicate that the
>> problem is on the VM side and not in OVS. Similarly maintaining
>> separate counters for all possible drops helps in indicating sensible
>> cause for packet drops.
>>
>> This patch adds custom software stats counters to track packets
>> dropped at port level and these counters are displayed along with
>> other stats in "ovs-vsctl get interface  statistics"
>> command. The detailed stats will be available for both dpdk and
>> vhostuser ports.
>>
>> Signed-off-by: Sriram Vatala 
>> ---
>> Changes since v7 :
>> Defined structure netdev_dpdk_sw_stats and moved all custom stats into
>> it.
>> Placed a pointer to netdev_dpdk_sw_stats structure in netdev_dpdk
>> strucure.
>> stats are reported with prefix with netdev_dpdk
>> ---
>>  include/openvswitch/netdev.h  |  14 +++
>>  lib/netdev-dpdk.c | 109 +++---
>>  utilities/bugtool/automake.mk |   3 +-
>>  utilities/bugtool/ovs-bugtool-get-port-stats  |  25 
>>  .../plugins/network-status/openvswitch.xml|   1 +
>>  vswitchd/vswitch.xml  |  24 
>>  6 files changed, 156 insertions(+), 20 deletions(-)  create mode
>> 100755 utilities/bugtool/ovs-bugtool-get-port-stats
>>
>> diff --git a/include/openvswitch/netdev.h
>> b/include/openvswitch/netdev.h index 0c10f7b48..c17e6a97d 100644
>> --- a/include/openvswitch/netdev.h
>> +++ b/include/openvswitch/netdev.h
>> @@ -89,6 +89,20 @@ struct netdev_stats {
>>  uint64_t rx_jabber_errors;
>>  };
>>
>> +/* Custom software stats for dpdk ports */ struct
>> +netdev_dpdk_sw_stats {
>> +/* No. of retries when unable to transmit. */
>> +uint64_t tx_retries;
>> +/* Pkt drops when unable to transmit; Probably Tx queue is full */
>> +uint64_t tx_failure_drops;
>> +/* Pkt len greater than max dev MTU */
>> +uint64_t tx_mtu_exceeded_drops;
>> +/* Pkt drops in egress policier processing */
>> +uint64_t tx_qos_drops;
>> +/* Pkt drops in ingress policier processing */
>> +uint64_t rx_qos_drops;
>> +};
> 
> This should not be in a common header since the only user is netdev-dpdk.c.
> Regarding this code itself:
> 1. s/policier/policer/
> 2. s/Pkt/Packet/, s/len/length/
> 3. s/max dev MTU/device MTU/ (MTU already has 'maximum' word).
> 4. All comments should end with a period.
> 
> Sorry I missed to check spell. I will fix this in my next patch v9
> I will move this structure definition to netdev-dpdk.c  and make it static.
> 
>> +
>>  /* Structure representation of custom statistics counter */  struct
>> netdev_custom_counter {
>>  uint64_t value;
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
>> bc20d6843..39aab4672 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -413,11 +413,10 @@ struct netdev_dpdk {
>>
>>  PADDED_MEMBERS(CACHE_LINE_SIZE,
>>  struct netdev_stats stats;
>> -/* Custom stat for retries when unable to transmit. */
>> -uint64_t tx_retries;
>> +struct netdev_dpdk_sw_stats *sw_stats;
>>  /* Protects stats */
>>  rte_spinlock_t stats_lock;
>> -/* 4 pad bytes here. */
>> +/* 36 pad bytes here. */
>>  );
>>
>>  PADDED_MEMBERS(CACHE_LINE_SIZE,
>> @@ -1171,7 +1170,8 @@ common_construct(struct netdev *netdev, dpdk_port_t 
>> port_no,
>>  dev->rte_xstats_ids = NULL;
>>  dev->rte_xstats_ids_size = 0;
>>
>> -dev->tx_retries = 0;
>> +dev->sw_stats = (struct netdev_dpdk_sw_stats *)
>> +xcalloc(1,sizeof(struct
>> + netdev_dpdk_sw_stats));
> 
> This should be:
>dev->sw_stats = xzalloc(sizeof *dev->sw_stats); Or
>dev->sw_stats = dpdk_rte_mzalloc(sizeof *dev->sw_stats);
> 
> The later variant will require proper error handling as it could return NULL.
> I will change this in next patch v9.
> 
> See the Documentation/internals/contributing/coding-style.rst for details.
> 
>>
>>  return 0;
>>  }
>> @@ -1357,6 +1357,7 @@ common_destruct(struct netdev_dpdk *dev)
>>  ovs_list_remove(>list_node);
>>  free(ovsrcu_get_protected(struct ingress_policer *,
>>

[ovs-dev] [PATCH] netdev-dpdk: Reuse vhost function for dpdk ETH custom stats.

[Ilya Maximets]

This is yet another refactoring for upcoming detailed drop stats.
It allowes to use single function for all the software calculated
statistics in netdev-dpdk for both vhost and ETH ports.

UINT64_MAX used as a marker for non-supported statistics in a
same way as it's done in bridge.c for common netdev stats.

Cc: Sriram Vatala 
Signed-off-by: Ilya Maximets 
---

Sriram, you could rebase your patch on top of this and send both
patches as a patch-set. So the changes will be logically splitted.

If you're using 'git format-patch' + 'git send-email' it should
preserve authorship correctly.

If you'll need some additional changes in this patch, you could
amend them along with mentioning yourself as co-author.

 lib/netdev-dpdk.c | 67 +++
 1 file changed, 39 insertions(+), 28 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index bc20d6843..652b57e3b 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -471,6 +471,8 @@ struct netdev_rxq_dpdk {
 static void netdev_dpdk_destruct(struct netdev *netdev);
 static void netdev_dpdk_vhost_destruct(struct netdev *netdev);
 
+static int netdev_dpdk_get_sw_custom_stats(const struct netdev *,
+   struct netdev_custom_stats *);
 static void netdev_dpdk_clear_xstats(struct netdev_dpdk *dev);
 
 int netdev_dpdk_get_vid(const struct netdev_dpdk *dev);
@@ -1171,7 +1173,7 @@ common_construct(struct netdev *netdev, dpdk_port_t 
port_no,
 dev->rte_xstats_ids = NULL;
 dev->rte_xstats_ids_size = 0;
 
-dev->tx_retries = 0;
+dev->tx_retries = (dev->type == DPDK_DEV_VHOST) ? 0 : UINT64_MAX;
 
 return 0;
 }
@@ -2771,7 +2773,9 @@ netdev_dpdk_get_custom_stats(const struct netdev *netdev,
 
 uint32_t i;
 struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
-int rte_xstats_ret;
+int rte_xstats_ret, sw_stats_size;
+
+netdev_dpdk_get_sw_custom_stats(netdev, custom_stats);
 
 ovs_mutex_lock(>mutex);
 
@@ -2786,12 +2790,13 @@ netdev_dpdk_get_custom_stats(const struct netdev 
*netdev,
 if (rte_xstats_ret > 0 &&
 rte_xstats_ret <= dev->rte_xstats_ids_size) {
 
-custom_stats->size = rte_xstats_ret;
-custom_stats->counters =
-(struct netdev_custom_counter *) xcalloc(rte_xstats_ret,
-sizeof(struct netdev_custom_counter));
+sw_stats_size = custom_stats->size;
+custom_stats->size += rte_xstats_ret;
+custom_stats->counters = xrealloc(custom_stats->counters,
+  custom_stats->size *
+  sizeof *custom_stats->counters);
 
-for (i = 0; i < rte_xstats_ret; i++) {
+for (i = sw_stats_size; i < sw_stats_size + rte_xstats_ret; i++) {
 ovs_strlcpy(custom_stats->counters[i].name,
 netdev_dpdk_get_xstat_name(dev,
dev->rte_xstats_ids[i]),
@@ -2801,8 +2806,6 @@ netdev_dpdk_get_custom_stats(const struct netdev *netdev,
 } else {
 VLOG_WARN("Cannot get XSTATS values for port: "DPDK_PORT_ID_FMT,
   dev->port_id);
-custom_stats->counters = NULL;
-custom_stats->size = 0;
 /* Let's clear statistics cache, so it will be
  * reconfigured */
 netdev_dpdk_clear_xstats(dev);
@@ -2817,39 +2820,47 @@ netdev_dpdk_get_custom_stats(const struct netdev 
*netdev,
 }
 
 static int
-netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev,
-   struct netdev_custom_stats *custom_stats)
+netdev_dpdk_get_sw_custom_stats(const struct netdev *netdev,
+struct netdev_custom_stats *custom_stats)
 {
 struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
-int i;
+int i, n;
 
-#define VHOST_CSTATS \
-VHOST_CSTAT(tx_retries)
+#define SW_CSTATS \
+SW_CSTAT(tx_retries)
 
-#define VHOST_CSTAT(NAME) + 1
-custom_stats->size = VHOST_CSTATS;
-#undef VHOST_CSTAT
+#define SW_CSTAT(NAME) + 1
+custom_stats->size = SW_CSTATS;
+#undef SW_CSTAT
 custom_stats->counters = xcalloc(custom_stats->size,
  sizeof *custom_stats->counters);
-i = 0;
-#define VHOST_CSTAT(NAME) \
-ovs_strlcpy(custom_stats->counters[i++].name, #NAME, \
-NETDEV_CUSTOM_STATS_NAME_SIZE);
-VHOST_CSTATS;
-#undef VHOST_CSTAT
 
 ovs_mutex_lock(>mutex);
 
 rte_spinlock_lock(>stats_lock);
 i = 0;
-#define VHOST_CSTAT(NAME) \
+#define SW_CSTAT(NAME) \
 custom_stats->counters[i++].value = dev->NAME;
-VHOST_CSTATS;
-#undef VHOST_CSTAT
+SW_CSTATS;
+#undef SW_CSTAT
 rte_spinlock_unlock(>stats_lock);
 
 ovs_mutex_unlock(>mutex);
 
+i = 0;
+n = 0;
+#define SW_CSTAT(NAME) \
+if (custom_stats->counters[i].value != UINT64_MAX) {  

[ovs-dev] [PATCH v3] netdev-dpdk: Fix flow control not configuring.

[Tomasz Konieczny]

Currently OVS is unable to change flow control configuration in DPDK
because new settings are being overwritten by current settings with
rte_eth_dev_flow_ctrl_get(). The fix restores correct order of
operations and at the same time does not trigger error on devices
without flow control support when flow control not requested.

Fixes: 7e1de65e8dfb ("netdev-dpdk: Fix failure to configure flow control at 
netdev-init.")
Signed-off-by: Tomasz Konieczny 
Co-authored-by: Ilya Maximets 
Signed-off-by: Ilya Maximets 
---
 lib/netdev-dpdk.c | 32 ++--
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index bc20d68..62985c5 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1771,6 +1771,7 @@ netdev_dpdk_set_config(struct netdev *netdev, const 
struct smap *args,
 {
 struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
 bool rx_fc_en, tx_fc_en, autoneg, lsc_interrupt_mode;
+bool flow_control_requested = true;
 enum rte_eth_fc_mode fc_mode;
 static const enum rte_eth_fc_mode fc_mode_set[2][2] = {
 {RTE_FC_NONE, RTE_FC_TX_PAUSE},
@@ -1858,15 +1859,34 @@ netdev_dpdk_set_config(struct netdev *netdev, const 
struct smap *args,
 autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false);
 
 fc_mode = fc_mode_set[tx_fc_en][rx_fc_en];
+
+if (!smap_get(args, "rx-flow-ctrl") && !smap_get(args, "tx-flow-ctrl")
+&& !smap_get(args, "flow-ctrl-autoneg")) {
+/* FIXME: User didn't ask for flow control configuration.
+ *For now we'll not print a warning if flow control is not
+ *supported by the DPDK port. */
+flow_control_requested = false;
+}
+
+/* Get the Flow control configuration. */
+err = rte_eth_dev_flow_ctrl_get(dev->port_id, >fc_conf);
+if (err) {
+if (err == -ENOTSUP) {
+if (flow_control_requested) {
+VLOG_WARN("%s: Flow control is not supported.",
+netdev_get_name(netdev));
+}
+err = 0; /* Not fatal. */
+} else {
+VLOG_WARN("%s: Cannot get flow control parameters: %s",
+  netdev_get_name(netdev), rte_strerror(-err));
+}
+goto out;
+}
+
 if (dev->fc_conf.mode != fc_mode || autoneg != dev->fc_conf.autoneg) {
 dev->fc_conf.mode = fc_mode;
 dev->fc_conf.autoneg = autoneg;
-/* Get the Flow control configuration for DPDK-ETH */
-err = rte_eth_dev_flow_ctrl_get(dev->port_id, >fc_conf);
-if (err) {
-VLOG_WARN("Cannot get flow control parameters on port "
-DPDK_PORT_ID_FMT", err=%d", dev->port_id, err);
-}
 dpdk_eth_flow_ctrl_setup(dev);
 }
 
-- 
2.7.4

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

Re: [ovs-dev] [PATCH v3] netdev-dpdk: add support for the RTE_ETH_EVENT_INTR_RESET event

[Ilya Maximets]

On 12.09.2019 13:19, Ilya Maximets wrote:
> On 12.09.2019 13:07, Eelco Chaudron wrote:
>>
>>
>> On 12 Sep 2019, at 10:39, Ilya Maximets wrote:
>>
>>> On 11.09.2019 16:20, Eelco Chaudron wrote:
 Currently, OVS does not register and therefore not handle the
 interface reset event from the DPDK framework. This would cause a
 problem in cases where a VF is used as an interface, and its
 configuration changes.

 As an example in the following scenario the MAC change is not
 detected/acted upon until OVS is restarted without the patch applied:

   $ echo 1 > /sys/bus/pci/devices/:05:00.1/sriov_numvfs
   $ ovs-vsctl add-port ovs_pvp_br0 dpdk0 -- \
     set Interface dpdk0 type=dpdk -- \
     set Interface dpdk0 options:dpdk-devargs=:05:0a.0

   $ ip link set p5p2 vf 0 mac 52:54:00:92:d3:33

 Signed-off-by: Eelco Chaudron 
 ---
>>>
>>> 
>>>
 @@ -4180,13 +4223,19 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
  && dev->rxq_size == dev->requested_rxq_size
  && dev->txq_size == dev->requested_txq_size
  && dev->socket_id == dev->requested_socket_id
 -    && dev->started) {
 +    && dev->started && !dev->reset_needed) {
  /* Reconfiguration is unnecessary */

  goto out;
  }

 -    rte_eth_dev_stop(dev->port_id);
 +    if (dev->reset_needed) {
 +    rte_eth_dev_reset(dev->port_id);
>>>
>>> Thinking more about the change and looking at flow control configuration,
>>> it seems that on reset we'll lost configurations done in set_config().
>>> Device reset should return it to initial state, i.e. all the default 
>>> settings,
>>> but set_config() will not be called after that.
>>> I know, that VFs commonly doesn't support flow control, but if we'll add 
>>> like
>>> real configuration of MAC address, it will be lost too.
>>>
>>> What do you think?
>>
>> Doesn’t the full bridge run sequence take care of this?
>>
>> So in callback we do netdev_request_reconfigure() which triggers the 
>> following sequence…
>>
>> bridge_run__()
>>  ...
>>    dpif_netdev_run()
>>  ...
>>    reconfigure_datapath()
>>  ...
>>    netdev_dpdk_reconfigure()
>>
>> But than also it will call in the next run
>>
>> bridge_run()
>>  ...
>>    bridge_delete_or_reconfigure_ports()
>>  ...
>>     netdev_set_config()
>>   netdev_dpdk_set_config()
>>
>>
>> Or do I miss something?
> 
> dpif_netdev_run() is called from ofproto_type_run() which is called
> unconditionally from the bridge_run().
> But bridge_delete_or_reconfigure_ports() only called from 
> bridge_reconfigure(),
> which is protected by the following condition:
> 
> if (ovsdb_idl_get_seqno(idl) != idl_seqno ||
> if_notifier_changed(ifnotifier)) {
> 
> i.e. if there will be no DB updates or there will be no netlink notifications,
> set_config() will not be called. I understand that in your scenario there
> might be netlink notification for interface changes since PF is controlled
> by the kernel, but it might be not the case if PF attached as a DPDK port
> to OVS or some other application.

Hmm. Basically, dpdk_eth_event_callback() is an analogue of the
if_notifier, but for DPDK application.

Maybe we can add it as another trigger for above 'if' condition?

> 
>>
>>>
>>> BTW, there is a discussion about flow control configuration:
>>> https://patchwork.ozlabs.org/patch/1159689/
>>>
 +    dev->reset_needed = false;
 +    } else {
 +    rte_eth_dev_stop(dev->port_id);
 +    }
 +
  dev->started = false;

  err = netdev_dpdk_mempool_configure(dev);
>>
>>
> 
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] netdev-dpdk: add support for the RTE_ETH_EVENT_INTR_RESET event

[Ilya Maximets]

On 12.09.2019 13:07, Eelco Chaudron wrote:
> 
> 
> On 12 Sep 2019, at 10:39, Ilya Maximets wrote:
> 
>> On 11.09.2019 16:20, Eelco Chaudron wrote:
>>> Currently, OVS does not register and therefore not handle the
>>> interface reset event from the DPDK framework. This would cause a
>>> problem in cases where a VF is used as an interface, and its
>>> configuration changes.
>>>
>>> As an example in the following scenario the MAC change is not
>>> detected/acted upon until OVS is restarted without the patch applied:
>>>
>>>   $ echo 1 > /sys/bus/pci/devices/:05:00.1/sriov_numvfs
>>>   $ ovs-vsctl add-port ovs_pvp_br0 dpdk0 -- \
>>>     set Interface dpdk0 type=dpdk -- \
>>>     set Interface dpdk0 options:dpdk-devargs=:05:0a.0
>>>
>>>   $ ip link set p5p2 vf 0 mac 52:54:00:92:d3:33
>>>
>>> Signed-off-by: Eelco Chaudron 
>>> ---
>>
>> 
>>
>>> @@ -4180,13 +4223,19 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
>>>  && dev->rxq_size == dev->requested_rxq_size
>>>  && dev->txq_size == dev->requested_txq_size
>>>  && dev->socket_id == dev->requested_socket_id
>>> -    && dev->started) {
>>> +    && dev->started && !dev->reset_needed) {
>>>  /* Reconfiguration is unnecessary */
>>>
>>>  goto out;
>>>  }
>>>
>>> -    rte_eth_dev_stop(dev->port_id);
>>> +    if (dev->reset_needed) {
>>> +    rte_eth_dev_reset(dev->port_id);
>>
>> Thinking more about the change and looking at flow control configuration,
>> it seems that on reset we'll lost configurations done in set_config().
>> Device reset should return it to initial state, i.e. all the default 
>> settings,
>> but set_config() will not be called after that.
>> I know, that VFs commonly doesn't support flow control, but if we'll add like
>> real configuration of MAC address, it will be lost too.
>>
>> What do you think?
> 
> Doesn’t the full bridge run sequence take care of this?
> 
> So in callback we do netdev_request_reconfigure() which triggers the 
> following sequence…
> 
> bridge_run__()
>  ...
>    dpif_netdev_run()
>  ...
>    reconfigure_datapath()
>  ...
>    netdev_dpdk_reconfigure()
> 
> But than also it will call in the next run
> 
> bridge_run()
>  ...
>    bridge_delete_or_reconfigure_ports()
>  ...
>     netdev_set_config()
>   netdev_dpdk_set_config()
> 
> 
> Or do I miss something?

dpif_netdev_run() is called from ofproto_type_run() which is called
unconditionally from the bridge_run().
But bridge_delete_or_reconfigure_ports() only called from bridge_reconfigure(),
which is protected by the following condition:

if (ovsdb_idl_get_seqno(idl) != idl_seqno ||
if_notifier_changed(ifnotifier)) {

i.e. if there will be no DB updates or there will be no netlink notifications,
set_config() will not be called. I understand that in your scenario there
might be netlink notification for interface changes since PF is controlled
by the kernel, but it might be not the case if PF attached as a DPDK port
to OVS or some other application.

> 
>>
>> BTW, there is a discussion about flow control configuration:
>> https://patchwork.ozlabs.org/patch/1159689/
>>
>>> +    dev->reset_needed = false;
>>> +    } else {
>>> +    rte_eth_dev_stop(dev->port_id);
>>> +    }
>>> +
>>>  dev->started = false;
>>>
>>>  err = netdev_dpdk_mempool_configure(dev);
> 
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] netdev-dpdk: add support for the RTE_ETH_EVENT_INTR_RESET event

[Eelco Chaudron]

On 12 Sep 2019, at 10:39, Ilya Maximets wrote:


On 11.09.2019 16:20, Eelco Chaudron wrote:

Currently, OVS does not register and therefore not handle the
interface reset event from the DPDK framework. This would cause a
problem in cases where a VF is used as an interface, and its
configuration changes.

As an example in the following scenario the MAC change is not
detected/acted upon until OVS is restarted without the patch applied:

  $ echo 1 > /sys/bus/pci/devices/:05:00.1/sriov_numvfs
  $ ovs-vsctl add-port ovs_pvp_br0 dpdk0 -- \
set Interface dpdk0 type=dpdk -- \
set Interface dpdk0 options:dpdk-devargs=:05:0a.0

  $ ip link set p5p2 vf 0 mac 52:54:00:92:d3:33

Signed-off-by: Eelco Chaudron 
---




@@ -4180,13 +4223,19 @@ netdev_dpdk_reconfigure(struct netdev 
*netdev)

 && dev->rxq_size == dev->requested_rxq_size
 && dev->txq_size == dev->requested_txq_size
 && dev->socket_id == dev->requested_socket_id
-&& dev->started) {
+&& dev->started && !dev->reset_needed) {
 /* Reconfiguration is unnecessary */

 goto out;
 }

-rte_eth_dev_stop(dev->port_id);
+if (dev->reset_needed) {
+rte_eth_dev_reset(dev->port_id);


Thinking more about the change and looking at flow control 
configuration,

it seems that on reset we'll lost configurations done in set_config().
Device reset should return it to initial state, i.e. all the default 
settings,

but set_config() will not be called after that.
I know, that VFs commonly doesn't support flow control, but if we'll 
add like

real configuration of MAC address, it will be lost too.

What do you think?


Doesn’t the full bridge run sequence take care of this?

So in callback we do netdev_request_reconfigure() which triggers the 
following sequence…


bridge_run__()
 ...
   dpif_netdev_run()
 ...
   reconfigure_datapath()
 ...
   netdev_dpdk_reconfigure()

But than also it will call in the next run

bridge_run()
 ...
   bridge_delete_or_reconfigure_ports()
 ...
netdev_set_config()
  netdev_dpdk_set_config()


Or do I miss something?



BTW, there is a discussion about flow control configuration:
https://patchwork.ozlabs.org/patch/1159689/


+dev->reset_needed = false;
+} else {
+rte_eth_dev_stop(dev->port_id);
+}
+
 dev->started = false;

 err = netdev_dpdk_mempool_configure(dev);

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

Re: [ovs-dev] [PATCH v2] netdev-dpdk: Fix flow control not configuring.

[0-day Robot]

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

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


checkpatch:
WARNING: Line is 81 characters long (recommended limit is 79)
#29 FILE: lib/netdev-dpdk.c:1773:
bool rx_fc_en, tx_fc_en, autoneg, lsc_interrupt_mode, 
flow_control_requested;

Lines checked: 77, Warnings: 1, Errors: 0


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

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

[ovs-dev] [PATCH v2] netdev-dpdk: Fix flow control not configuring.

[Tomasz Konieczny]

Currently OVS is unable to change flow control configuration in DPDK
because new settings are being overwritten by current settings with
rte_eth_dev_flow_ctrl_get(). The fix restores correct order of
operations and at the same time does not trigger error on devices
without flow control support when flow control not requested.

Fixes: 7e1de65e8dfb ("netdev-dpdk: Fix failure to configure flow control at 
netdev-init.")
Signed-off-by: Tomasz Konieczny 
Co-authored-by: Ilya Maximets 
Signed-off-by: Ilya Maximets 
---
Version history: [PATCH v1] netdev-dpdk: Fix flow control configuration.
---
 lib/netdev-dpdk.c | 34 +++---
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index bc20d68..942bb86 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1770,7 +1770,7 @@ netdev_dpdk_set_config(struct netdev *netdev, const 
struct smap *args,
char **errp)
 {
 struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
-bool rx_fc_en, tx_fc_en, autoneg, lsc_interrupt_mode;
+bool rx_fc_en, tx_fc_en, autoneg, lsc_interrupt_mode, 
flow_control_requested;
 enum rte_eth_fc_mode fc_mode;
 static const enum rte_eth_fc_mode fc_mode_set[2][2] = {
 {RTE_FC_NONE, RTE_FC_TX_PAUSE},
@@ -1858,15 +1858,35 @@ netdev_dpdk_set_config(struct netdev *netdev, const 
struct smap *args,
 autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false);
 
 fc_mode = fc_mode_set[tx_fc_en][rx_fc_en];
+
+flow_control_requested = true;
+if (!smap_get(args, "rx-flow-ctrl") && !smap_get(args, "tx-flow-ctrl")
+&& !smap_get(args, "flow-ctrl-autoneg")) {
+/* FIXME: User didn't ask for flow control configuration.
+ *For now we'll not print a warning if flow control is not
+ *supported by the DPDK port. */
+flow_control_requested = false;
+}
+
+/* Get the Flow control configuration. */
+err = rte_eth_dev_flow_ctrl_get(dev->port_id, >fc_conf);
+if (err) {
+if (err == -ENOTSUP) {
+if (flow_control_requested) {
+VLOG_WARN("%s: Flow control is not supported.",
+netdev_get_name(netdev));
+}
+err = 0; /* Not fatal. */
+} else {
+VLOG_WARN("%s: Cannot get flow control parameters: %s",
+  netdev_get_name(netdev), rte_strerror(-err));
+}
+goto out;
+}
+
 if (dev->fc_conf.mode != fc_mode || autoneg != dev->fc_conf.autoneg) {
 dev->fc_conf.mode = fc_mode;
 dev->fc_conf.autoneg = autoneg;
-/* Get the Flow control configuration for DPDK-ETH */
-err = rte_eth_dev_flow_ctrl_get(dev->port_id, >fc_conf);
-if (err) {
-VLOG_WARN("Cannot get flow control parameters on port "
-DPDK_PORT_ID_FMT", err=%d", dev->port_id, err);
-}
 dpdk_eth_flow_ctrl_setup(dev);
 }
 
-- 
2.7.4

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

[ovs-dev] [PATCH v16 08/14] netdev-dpdk: support multi-segment jumbo frames.

[Michal Obrembski]

From: Mark Kavanagh 

Currently, jumbo frame support for OvS-DPDK is implemented by
increasing the size of mbufs within a mempool, such that each mbuf
within the pool is large enough to contain an entire jumbo frame of
a user-defined size. Typically, for each user-defined MTU,
'requested_mtu', a new mempool is created, containing mbufs of size
~requested_mtu.

With the multi-segment approach, a port uses a single mempool,
(containing standard/default-sized mbufs of ~2k bytes), irrespective
of the user-requested MTU value. To accommodate jumbo frames, mbufs
are chained together, where each mbuf in the chain stores a portion of
the jumbo frame. Each mbuf in the chain is termed a segment, hence the
name.

== Enabling multi-segment mbufs ==
Multi-segment and single-segment mbufs are mutually exclusive, and the
user must decide on which approach to adopt on init. The introduction
of a new OVSDB field, 'dpdk-multi-seg-mbufs', facilitates this. This
is a global boolean value, which determines how jumbo frames are
represented across all DPDK ports. In the absence of a user-supplied
value, 'dpdk-multi-seg-mbufs' defaults to false, i.e. multi-segment
mbufs must be explicitly enabled / single-segment mbufs remain the
default.

Setting the field is identical to setting existing DPDK-specific OVSDB
fields:

ovs-vsctl set Open_vSwitch . other_config:dpdk-init=true
ovs-vsctl set Open_vSwitch . other_config:dpdk-lcore-mask=0x10
ovs-vsctl set Open_vSwitch . other_config:dpdk-socket-mem=4096,0
==> ovs-vsctl set Open_vSwitch . other_config:dpdk-multi-seg-mbufs=true

Co-authored-by: Tiago Lam 

Signed-off-by: Mark Kavanagh 
Signed-off-by: Tiago Lam 
Acked-by: Eelco Chaudron 
---
 Documentation/topics/dpdk/jumbo-frames.rst | 73 ++
 Documentation/topics/dpdk/memory.rst   | 36 +++
 NEWS   |  1 +
 lib/dpdk.c |  8 
 lib/netdev-dpdk.c  | 68 +---
 lib/netdev-dpdk.h  |  1 +
 vswitchd/vswitch.xml   | 22 +
 7 files changed, 202 insertions(+), 7 deletions(-)

diff --git a/Documentation/topics/dpdk/jumbo-frames.rst 
b/Documentation/topics/dpdk/jumbo-frames.rst
index 00360b4..9804bbb 100644
--- a/Documentation/topics/dpdk/jumbo-frames.rst
+++ b/Documentation/topics/dpdk/jumbo-frames.rst
@@ -71,3 +71,76 @@ Jumbo frame support has been validated against 9728B frames, 
which is the
 largest frame size supported by Fortville NIC using the DPDK i40e driver, but
 larger frames and other DPDK NIC drivers may be supported. These cases are
 common for use cases involving East-West traffic only.
+
+---
+Multi-segment mbufs
+---
+
+Instead of increasing the size of mbufs within a mempool, such that each mbuf
+within the pool is large enough to contain an entire jumbo frame of a
+user-defined size, mbufs can be chained together instead. In this approach each
+mbuf in the chain stores a portion of the jumbo frame, by default ~2K bytes,
+irrespective of the user-requested MTU value. Since each mbuf in the chain is
+termed a segment, this approach is named "multi-segment mbufs".
+
+This approach may bring more flexibility in use cases where the maximum packet
+length may be hard to guess. For example, in cases where packets originate from
+sources marked for offload (such as TSO), each packet may be larger than the
+MTU, and as such, when forwarding it to a DPDK port a single mbuf may not be
+enough to hold all of the packet's data.
+
+Multi-segment and single-segment mbufs are mutually exclusive, and the user
+must decide on which approach to adopt on initialisation. If multi-segment
+mbufs is to be enabled, it can be done so with the following command::
+
+$ ovs-vsctl set Open_vSwitch . other_config:dpdk-multi-seg-mbufs=true
+
+Single-segment mbufs still remain the default when using OvS-DPDK, and the
+above option `dpdk-multi-seg-mbufs` must be explicitly set to `true` if
+multi-segment mbufs are to be used.
+
+~
+Performance notes
+~
+
+When using multi-segment mbufs some PMDs may not support vectorized Tx
+functions, due to its non-contiguous nature. As a result this can hit
+performance for smaller packet sizes. For example, on a setup sending 64B
+packets at line rate, a decrease of ~20% has been observed. The performance
+impact stops being noticeable for larger packet sizes, although the exact size
+will depend on each PMD, and vary between architectures.
+
+Tests performed with the i40e PMD driver only showed this limitation for 64B
+packets, and the same rate was observed when comparing multi-segment mbufs and
+single-segment mbuf for 128B packets. In other words, the 20% drop in
+performance was not observed for packets >= 128B during this test case.
+
+Because of this, multi-segment mbufs is not advised to be used with smaller
+packet sizes, such as 

[ovs-dev] [PATCH v16 07/14] netdev-dpdk: copy large packet to multi-seg. mbufs

[Michal Obrembski]

From: Mark Kavanagh 

Currently, packets are only copied to a single segment in the function
dpdk_do_tx_copy(). This could be an issue in the case of jumbo frames,
particularly when multi-segment mbufs are involved.

This patch calculates the number of segments needed by a packet and
copies the data to each segment.

A new function, dpdk_buf_alloc(), has also been introduced as a wrapper
around the nonpmd_mp_mutex to serialise allocations from a non-pmd
context.

Co-authored-by: Michael Qiu 
Co-authored-by: Tiago Lam 

Signed-off-by: Mark Kavanagh 
Signed-off-by: Michael Qiu 
Signed-off-by: Tiago Lam 
Acked-by: Eelco Chaudron 
---
 lib/netdev-dpdk.c | 89 +--
 1 file changed, 80 insertions(+), 9 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 6fef910..6b66fc3 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -528,6 +528,25 @@ dpdk_rte_mzalloc(size_t sz)
 return rte_zmalloc(OVS_VPORT_DPDK, sz, OVS_CACHE_LINE_SIZE);
 }
 
+static struct rte_mbuf *
+dpdk_buf_alloc(struct rte_mempool *mp)
+{
+struct rte_mbuf *mbuf = NULL;
+
+/* If non-pmd we need to lock on nonpmd_mp_mutex mutex. */
+if (dpdk_thread_is_pmd()) {
+mbuf = rte_pktmbuf_alloc(mp);
+} else {
+ovs_mutex_lock(_mp_mutex);
+
+mbuf = rte_pktmbuf_alloc(mp);
+
+ovs_mutex_unlock(_mp_mutex);
+}
+
+return mbuf;
+}
+
 void
 free_dpdk_buf(struct dp_packet *p)
 {
@@ -2424,6 +2443,56 @@ out:
 }
 }
 
+static int
+dpdk_copy_dp_packet_to_mbuf(struct dp_packet *packet, struct rte_mbuf **head,
+struct rte_mempool *mp)
+{
+struct rte_mbuf *mbuf, *fmbuf;
+uint16_t max_data_len;
+uint32_t nb_segs = 0;
+uint32_t size = 0;
+
+/* We will need the whole data for copying below. */
+if (!dp_packet_is_linear(packet)) {
+dp_packet_linearize(packet);
+}
+
+/* Allocate first mbuf to know the size of data available. */
+fmbuf = mbuf = *head = dpdk_buf_alloc(mp);
+if (OVS_UNLIKELY(!mbuf)) {
+return ENOMEM;
+}
+
+size = dp_packet_size(packet);
+
+/* All new allocated mbuf's max data len is the same. */
+max_data_len = mbuf->buf_len - mbuf->data_off;
+
+/* Calculate # of output mbufs. */
+nb_segs = size / max_data_len;
+if (size % max_data_len) {
+nb_segs = nb_segs + 1;
+}
+
+/* Allocate additional mbufs, less the one alredy allocated above. */
+for (int i = 1; i < nb_segs; i++) {
+mbuf->next = dpdk_buf_alloc(mp);
+if (!mbuf->next) {
+free_dpdk_buf(CONTAINER_OF(fmbuf, struct dp_packet, mbuf));
+fmbuf = NULL;
+return ENOMEM;
+}
+mbuf = mbuf->next;
+}
+
+fmbuf->nb_segs = nb_segs;
+fmbuf->pkt_len = size;
+
+dp_packet_mbuf_write(fmbuf, 0, size, dp_packet_data(packet));
+
+return 0;
+}
+
 /* Tx function. Transmit packets indefinitely */
 static void
 dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch *batch)
@@ -2440,6 +2509,7 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct 
dp_packet_batch *batch)
 struct rte_mbuf *pkts[PKT_ARRAY_SIZE];
 uint32_t cnt = batch_cnt;
 uint32_t dropped = 0;
+uint32_t i;
 
 if (dev->type != DPDK_DEV_VHOST) {
 /* Check if QoS has been configured for this netdev. */
@@ -2450,28 +2520,29 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct 
dp_packet_batch *batch)
 
 uint32_t txcnt = 0;
 
-for (uint32_t i = 0; i < cnt; i++) {
+for (i = 0; i < cnt; i++) {
 struct dp_packet *packet = batch->packets[i];
 uint32_t size = dp_packet_size(packet);
+int err = 0;
 
 if (OVS_UNLIKELY(size > dev->max_packet_len)) {
 VLOG_WARN_RL(, "Too big size %u max_packet_len %d",
  size, dev->max_packet_len);
-
 dropped++;
 continue;
 }
 
-pkts[txcnt] = rte_pktmbuf_alloc(dev->dpdk_mp->mp);
-if (OVS_UNLIKELY(!pkts[txcnt])) {
+err = dpdk_copy_dp_packet_to_mbuf(packet, [txcnt],
+  dev->dpdk_mp->mp);
+if (err != 0) {
+if (err == ENOMEM) {
+VLOG_ERR_RL(, "Failed to alloc mbufs! %u packets dropped",
+cnt - i);
+}
+
 dropped += cnt - i;
 break;
 }
-
-/* We have to do a copy for now */
-memcpy(rte_pktmbuf_mtod(pkts[txcnt], void *),
-   dp_packet_data(packet), size);
-dp_packet_set_size((struct dp_packet *)pkts[txcnt], size);
 dp_packet_copy_mbuf_flags((struct dp_packet *)pkts[txcnt], packet);
 
 txcnt++;
-- 
2.7.4

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

[ovs-dev] [PATCH v16 06/14] dp-packet: Add support for data "linearization".

[Michal Obrembski]

From: Tiago Lam 

Previous commits have added support to the dp_packet API to handle
multi-segmented packets, where data is not stored contiguously in
memory. However, in some cases, it is inevitable and data must be
provided contiguously. Examples of such cases are when performing csums
over the entire packet data, or when write()'ing to a file descriptor
(for a tap interface, for example). For such cases, the dp_packet API
has been extended to provide a way to transform a multi-segmented
DPBUF_DPDK packet into a DPBUF_MALLOC system packet (at the expense of a
copy of memory). If the packet's data is already stored in memory
contigously then there's no need to convert the packet.

Thus, the main use cases that were assuming that a dp_packet's data is
always held contiguously in memory were changed to make use of the new
"linear functions" in the dp_packet API when there's a need to traverse
the entire's packet data. Per the example above, when the packet's data
needs to be write() to the tap's file descriptor, or when the conntrack
module needs to verify a packet's checksum, the data is now linearized.

Additionally, the miniflow_extract() function has been modified to check
if the respective packet headers don't span across multiple mbufs. This
requirement is needed to guarantee that callers can assume headers are
always in contiguous memory.

Signed-off-by: Tiago Lam 
---
 lib/conntrack.c   |   5 ++
 lib/crc32c.c  |  17 +++-
 lib/crc32c.h  |   2 +
 lib/dp-packet.c   |  18 
 lib/dp-packet.h   | 197 +-
 lib/dpif-netdev.c |  18 +++-
 lib/dpif-netlink.c|   3 +
 lib/dpif.c|   6 ++
 lib/flow.c| 111 
 lib/flow.h|   4 +-
 lib/mcast-snooping.c  |   2 +
 lib/netdev-bsd.c  |   3 +
 lib/netdev-dummy.c|   6 ++
 lib/netdev-linux.c|   6 ++
 lib/netdev-native-tnl.c   |  26 +++---
 lib/odp-execute.c |  24 -
 lib/packets.c |  96 +---
 lib/packets.h |   7 ++
 ofproto/ofproto-dpif-upcall.c |  21 +++--
 ofproto/ofproto-dpif-xlate.c  |  27 +-
 tests/test-rstp.c |   9 +-
 tests/test-stp.c  |   9 +-
 22 files changed, 529 insertions(+), 88 deletions(-)

diff --git a/lib/conntrack.c b/lib/conntrack.c
index e5266e5..9976546 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -1211,6 +1211,11 @@ conntrack_execute(struct conntrack *ct, struct 
dp_packet_batch *pkt_batch,
 struct conn_lookup_ctx ctx;
 
 DP_PACKET_BATCH_FOR_EACH (i, packet, pkt_batch) {
+/* Linearize the packet to ensure conntrack has the whole data */
+if (!dp_packet_is_linear(packet)) {
+dp_packet_linearize(packet);
+}
+
 if (packet->md.ct_state == CS_INVALID
 || !conn_key_extract(ct, packet, dl_type, , zone)) {
 packet->md.ct_state = CS_INVALID;
diff --git a/lib/crc32c.c b/lib/crc32c.c
index e8dd6ee..83beec7 100644
--- a/lib/crc32c.c
+++ b/lib/crc32c.c
@@ -141,19 +141,30 @@ ovs_be32
 crc32c(const uint8_t *data, size_t size)
 {
 uint32_t crc = 0xL;
+return crc32c_finish(crc32c_continue(crc, data, size));
+}
 
+uint32_t
+crc32c_continue(uint32_t partial, const uint8_t *data, size_t size)
+{
 while (size--) {
-crc = crc32Table[(crc ^ *data++) & 0xff] ^ (crc >> 8);
+partial = crc32Table[(partial ^ *data++) & 0xff] ^ (partial >> 8);
 }
 
+return partial;
+}
+
+ovs_be32
+crc32c_finish(uint32_t partial)
+{
 /* The result of this CRC calculation provides us a value in the reverse
  * byte-order as compared with our architecture. On big-endian systems,
  * this is opposite to our return type. So, to return a big-endian
  * value, we must swap the byte-order. */
 #if defined(WORDS_BIGENDIAN)
-crc = uint32_byteswap(crc);
+crc = uint32_byteswap(partial);
 #endif
 
 /* Our value is in network byte-order. OVS_FORCE keeps sparse happy. */
-return (OVS_FORCE ovs_be32) ~crc;
+return (OVS_FORCE ovs_be32) ~partial;
 }
diff --git a/lib/crc32c.h b/lib/crc32c.h
index 92c7d7f..17c8190 100644
--- a/lib/crc32c.h
+++ b/lib/crc32c.h
@@ -20,6 +20,8 @@
 
 #include "openvswitch/types.h"
 
+uint32_t crc32c_continue(uint32_t partial, const uint8_t *data, size_t size);
+ovs_be32 crc32c_finish(uint32_t partial);
 ovs_be32 crc32c(const uint8_t *data, size_t);
 
 #endif /* crc32c.h */
diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 0707bb4..9aacec9 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -121,6 +121,9 @@ void
 dp_packet_init_dpdk(struct dp_packet *b)
 {
 b->source = DPBUF_DPDK;
+#ifdef DPDK_NETDEV
+b->mstate = NULL;
+#endif
 }
 
 /* Initializes 'b' as an empty dp_packet with an initial capacity of 'size'
@@ -138,6 +141,21 @@ dp_packet_uninit(struct dp_packet *b)
 

Re: [ovs-dev] [PATCH] MAINTAINERS: Update email for Ilya Maximets.

[Ilya Maximets]

On 11.09.2019 20:34, Ben Pfaff wrote:
> On Wed, Sep 11, 2019 at 06:51:44PM +0300, Ilya Maximets wrote:
>> CC: Ilya Maximets 
>> Signed-off-by: Ilya Maximets 
>> ---
>>
>> As someone could already know, my samsung email will be deactivated
>> soon. So, the ovn.org address should be used instead.
>>
>> BTW, I'll continue working with upstream OVS, i.e. patch review,
>> preparing fixes. Stay tuned!
>> However, I might be unavailable on next week due to DPDK Summit.
> 
> Hope all is well.

Yes. I'm just relocating myself out of Moscow.

> 
> Acked-by: Ben Pfaff 

Thanks! Applied to master.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] netdev-dpdk: add support for the RTE_ETH_EVENT_INTR_RESET event

[Ilya Maximets]

On 11.09.2019 16:20, Eelco Chaudron wrote:
> Currently, OVS does not register and therefore not handle the
> interface reset event from the DPDK framework. This would cause a
> problem in cases where a VF is used as an interface, and its
> configuration changes.
> 
> As an example in the following scenario the MAC change is not
> detected/acted upon until OVS is restarted without the patch applied:
> 
>   $ echo 1 > /sys/bus/pci/devices/:05:00.1/sriov_numvfs
>   $ ovs-vsctl add-port ovs_pvp_br0 dpdk0 -- \
> set Interface dpdk0 type=dpdk -- \
> set Interface dpdk0 options:dpdk-devargs=:05:0a.0
> 
>   $ ip link set p5p2 vf 0 mac 52:54:00:92:d3:33
> 
> Signed-off-by: Eelco Chaudron 
> ---



> @@ -4180,13 +4223,19 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
>  && dev->rxq_size == dev->requested_rxq_size
>  && dev->txq_size == dev->requested_txq_size
>  && dev->socket_id == dev->requested_socket_id
> -&& dev->started) {
> +&& dev->started && !dev->reset_needed) {
>  /* Reconfiguration is unnecessary */
>  
>  goto out;
>  }
>  
> -rte_eth_dev_stop(dev->port_id);
> +if (dev->reset_needed) {
> +rte_eth_dev_reset(dev->port_id);

Thinking more about the change and looking at flow control configuration,
it seems that on reset we'll lost configurations done in set_config().
Device reset should return it to initial state, i.e. all the default settings,
but set_config() will not be called after that.
I know, that VFs commonly doesn't support flow control, but if we'll add like
real configuration of MAC address, it will be lost too.

What do you think?

BTW, there is a discussion about flow control configuration:
https://patchwork.ozlabs.org/patch/1159689/

> +dev->reset_needed = false;
> +} else {
> +rte_eth_dev_stop(dev->port_id);
> +}
> +
>  dev->started = false;
>  
>  err = netdev_dpdk_mempool_configure(dev);
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH branch-2.12] pinctrl: Fix DNS packet parsing

[Dumitru Ceara]

On Wed, Aug 28, 2019 at 9:39 PM Ben Pfaff  wrote:
>
> On Fri, Aug 16, 2019 at 04:35:52PM +0200, Dumitru Ceara wrote:
> > Please backport this to 2.11, 2.10, 2.9 at least.
>
> Done.

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

Re: [ovs-dev] [PATCH v1] netdev-dpdk: Fix flow control configuration.

[Konieczny, TomaszX]

>-Original Message-
>From: Ilya Maximets 
>Sent: 11 September 2019 15:29
>To: Konieczny, TomaszX ; d...@openvswitch.org
>Cc: Stokes, Ian 
>Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Fix flow control configuration.
>
>OK. We could, actually, check if user configured something for flow control in
>following way:
>
>flow_control_requested = true;
>if (!smap_get(args, "rx-flow-ctrl") && !smap_get(args, "tx-flow-ctrl")
>&& !smap_get(args, "flow-ctrl-autoneg")) {
>/* FIXME: User didn't ask for flow control configuration.
> *For now we'll not print a warning if flow control is not
> *supported by the DPDK port. */
>flow_control_requested = false;
>}
>
>And warn only if something was requested:
>
>if (err == -ENOTSUP) {
>if (flow_control_requested) {
>VLOG_WARN("%s: Flow control is not supported.",
>  netdev_get_name(netdev));
>}
>err = 0; /* Not fatal. */
> }
>
>Alternatively we could dynamically choose the log level in case we want some
>message printed in both cases:
>
>  VLOG(flow_control_requested ? VLL_WARN : VLL_INFO,
>   "%s: Flow control is not supported.", netdev_get_name(netdev));
>
>VLL_INFO could be changed to VLL_DBG if you think we shouldn't bother users
>with additional information.
>
>Best regards, Ilya Maximets.

Ok, that should work, I'll prepare v2.

Regards
Tomasz Konieczny
-
Intel Corporation (UK) Ltd.
Co. Reg. #1134945
Pipers Way, Swindon SN3 1RJ
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] Learn the mac binding only if required

[Han Zhou]

On Wed, Sep 11, 2019 at 1:05 PM  wrote:
>
> From: Numan Siddique 
>
> OVN has the actions - put_arp and put_nd to learn the mac bindings from
the
> ARP/ND packets. These actions update the Southbound MAC_Binding table.
> These actions translates to controller actions. Whenever pinctrl thread
> receives such packets, it wakes up the main ovn-controller thread.
> If the MAC_Binding table is already upto date, this results
> in unnecessary CPU cyles. There are some security implications as well.
> A rogue VM can flood broadcast ARP request/reply packets and this
> could cause DoS issues. A physical switch may send periodic GARPs
> and these packets hit ovn-controllers.
>
> This patch solves these problems by learning the mac bindings only if
> required. There is no need to apply the put_arp/put_nd action if the
> Southbound MAC_Binding row is upto date.
>
> A new action - lookup_arp and lookup_nd is added which looks up the
> IP, MAC pair in the mac_binding table and updates the eth.dst if
> the entry is present, else eth.dst is set to 00:00:00:00:00:00.
>
> ovn-northd adds 2 new stages - lookup_arp and put_arp before ip_input
> in the router ingress pipeline.
>
> The logical flows looks something like:
>
> table=1 (lr_in_lookup_arp), priority=100  , match=(arp),
>  action=(xxreg1[0..47] = eth.dst;
>  lookup_arp(inport, arp.spa, arp.sha);
>  xxreg0[0..47] = eth.dst; eth.dst = xxreg1[0..47]; next;)
>
> table=1 (lr_in_lookup_arp), priority=0, match=(1), action=(next;)
> ...
> table=2 (lr_in_put_arp   ), priority=100  ,
>  match=(arp.op == 2 && xxreg0[0..47] == 00:00:00:00:00:00),
>  action=(put_arp(inport, arp.spa, arp.sha);)
> table=2 (lr_in_put_arp   ), priority=90   , match=(arp.op == 2),
action=(drop;)
> table=2 (lr_in_put_arp   ), priority=0, match=(1), action=(next;)
>
> The lflow module of ovn-controller adds OF flows in table 31
(OFTABLE_MAC_LOOKUP)
> for each mac_binding entry with the match reg0 = ip && eth.src = mac with
> the action - eth.dst = mac
>
> Eg:
> table=31,
priority=100,arp,reg0=0xaca8006f,reg14=0x3,metadata=0x3,dl_src=00:44:00:00:00:04
>   actions=mod_dl_dst:00:44:00:00:00:04
>
> This patch should also address the issue reported in 'Reported-at'
>
> Reported-at: https://bugzilla.redhat.com/1729846
> Reported-by: Haidong Li 
> CC: Han ZHou 
> CC: Dumitru Ceara 
> Signed-off-by: Numan Siddique 

Thanks Numan for optimizing this. First of all, this approach looks good to
me. I haven't finished the review yet, but have a quick question. Why the
action lookup_arp() need to store the mac in eth.dst? I thought it would be
more straightforward if it sets 1 in a register if matched, and 0 if no
match. Is there any limit of OpenFlow for doing that?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev