Re: [ovs-dev] [PATCH v3] dpif-netdev: Allow PMD auto load balance with cross-numa.

[Yogananth Subramanian]

Hello Kevin
Thanks for the review of the results.
It seems the tables sent from gmail are not formatted correctly, as you
have suggested will use plain text or mail client that sends in plain text,
so that things are formatted the right way.

On Wed, May 5, 2021 at 6:02 PM Kevin Traynor  wrote:

> On 04/05/2021 17:12, Yogananth Subramanian wrote:
> > Hello Everyone
>
> Hi Yogananth,
>
> > I was able to verify the patch, below are the  observed behavior of  OVS
> > 2.15 release and OVS from git with the patch.
> > OVS 2.15 release :
> >
> > PMD auto balance aborted:
> >
> > 2021-04-27T08:54:54.770Z|00406|dpif_netdev|DBG|PMD auto lb dry run.
> > Current: Core 3, usage 51
> >
> > 2021-04-27T08:54:54.770Z|00407|dpif_netdev|DBG|PMD auto lb dry run.
> > Current: Core 5, usage 51
> >
> > 2021-04-27T08:54:54.770Z|00408|dpif_netdev|DBG|PMD auto lb dry run.
> > Current: Core 27, usage 96
> >
> > 2021-04-27T08:54:54.770Z|00409|dpif_netdev|DBG|PMD auto lb dry run.
> > Current: Core 29, usage 93
> >
> > 2021-04-27T08:54:54.770Z|00410|dpif_netdev|DBG|PMD auto lb dry run.
> > Aborting due to cross-numa polling.
>
> ^ right, this is where without the patch any cross-numa polling prevents
> the alb feature from working.
>
> > OVS from git with PDM cross numa rebalance commit:
> >
> > PMD auto balance triggered:
> >
> > 2021-04-27T11:32:17.985Z|00805|dpif_netdev|INFO|PMD auto lb dry run.
> > requesting datapath reconfigure.
> >
> > 2021-04-27T11:32:17.985Z|00806|dpif_netdev|WARN|There's no available
> > (non-isolated) pmd thread on numa node 0. Queue 0 on port 'dpdk-p1' will
> be
> > assigned to the pmd on core 3 (numa node 1). Expect reduced performance.
> >
> > 2021-04-27T11:32:17.985Z|00807|dpif_netdev|WARN|There's no available
> > (non-isolated) pmd thread on numa node 0. Queue 0 on port 'dpdk-p0' will
> be
> > assigned to the pmd on core 29 (numa node 1). Expect reduced performance.
> >
> > 2021-04-27T11:32:17.985Z|00808|dpif_netdev|WARN|There's no available
> > (non-isolated) pmd thread on numa node 0. Queue 2 on port 'dpdk-p0' will
> be
> > assigned to the pmd on core 27 (numa node 1). Expect reduced performance.
> >
> > 2021-04-27T11:32:17.985Z|00809|dpif_netdev|WARN|There's no available
> > (non-isolated) pmd thread on numa node 0. Queue 1 on port 'dpdk-p0' will
> be
> > assigned to the pmd on core 5 (numa node 1). Expect reduced performance.
> >
> > 2021-04-27T11:32:17.985Z|00810|dpif_netdev|WARN|There's no available
> > (non-isolated) pmd thread on numa node 0. Queue 2 on port 'dpdk-p1' will
> be
> > assigned to the pmd on core 5 (numa node 1). Expect reduced performance.
> >
> > 2021-04-27T11:32:17.985Z|00811|dpif_netdev|WARN|There's no available
> > (non-isolated) pmd thread on numa node 0. Queue 1 on port 'dpdk-p1' will
> be
> > assigned to the pmd on core 27 (numa node 1). Expect reduced performance.
> >
>
> ^ yes, this indicates that the alb has triggered a rebalance while there
> is cross-numa polling limited to pmds on one numa node, which is the
> intended behaviour.
>
> btw, just to note for future mails that plaintext is preferred on the
> mailing list.
>
> Thanks for testing,
> Kevin.
>
> >
> > Before rebalance
> >
> > After rebalance
> >
> > pmd thread numa_id 1 core_id 3:
> >
> >   isolated : false
> >
> >   port: dpdk-p1   queue-id:  0 (enabled)   pmd usage: 46 %
> >
> > pmd thread numa_id 1 core_id 5:
> >
> >   isolated : false
> >
> >   port: dpdk-p0   queue-id:  0 (enabled)   pmd usage: 32 %
> >
> >   port: dpdk-p0   queue-id:  1 (enabled)   pmd usage: 26 %
> >
> > pmd thread numa_id 1 core_id 27:
> >
> >   isolated : false
> >
> >   port: dpdk-p0   queue-id:  2 (enabled)   pmd usage: 35 %
> >
> >   port: dpdk-p1   queue-id:  2 (enabled)   pmd usage: 18 %
> >
> > pmd thread numa_id 1 core_id 29:
> >
> >   isolated : false
> >
> >   port: dpdk-p1   queue-id:  1 (enabled)   pmd usage: 13 %
> >
> > pmd thread numa_id 1 core_id 3:
> >
> >   isolated : false
> >
> >   port: dpdk-p1   queue-id:  0 (enabled)   pmd usage: 76 %
> >
> > pmd thread numa_id 1 core_id 5:
> >
> >   isolated : false
> >
> >   port: dpdk-p0   queue-id:  1 (enabled)   pmd usage: 47 %
> >
> >   port: dpdk-p1   queue-id:  2 (enabled)   pmd usage: 23 %
> >
> > pmd thread numa_id 1 core_id 27:
> >
> >   isolated : false
> >
> >   port: dpdk-p0   queue-id:  0 (enabled)   pmd usage: 67 %
> >
> >   port: dpdk-p1   queue-id:  1 (enabled)   pmd usage: 19 %
> >
> > pmd thread numa_id 1 core_id 29:
> >
> >   isolated : false
> >
> >   port: dpdk-p0   queue-id:  2 (enabled)   pmd usage: 54 %
> >
> >
> >
> > On Mon, Mar 22, 2021 at 5:31 PM Ilya Maximets 
> wrote:
> >
> >> On 3/18/21 12:34 PM, Kevin Traynor wrote:
> >>> Previously auto load balance did not trigger a reassignment when
> >>> there was any cross-numa polling as an rxq could be polled from a
> >>> different numa after reassign and it could impact estimates.
> >>>
> >>> In the case 

Re: [ovs-dev] [PATCH v8 ovn] ovn-northd: introduce new allow-stateless ACL verb

[0-day Robot]

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

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


build:
  /bin/python3 ./build-aux/dpdkstrip.py  | \
  sed \
-e 's,[@]PKIDIR[@],/usr/local/var/lib/openvswitch/pki,g' \
-e 's,[@]LOGDIR[@],/usr/local/var/log/ovn,g' \
-e 's,[@]DBDIR[@],/usr/local/etc/ovn,g' \
-e 's,[@]PYTHON3[@],/bin/python3,g' \
-e 's,[@]OVN_RUNDIR[@],/usr/local/var/run/ovn,g' \
-e 
's,[@]OVSBUILDDIR[@],/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR,g'
 \
-e 's,[@]VERSION[@],21.03.90,g' \
-e 's,[@]OVSVERSION[@],2.15.90,g' \
-e 's,[@]localstatedir[@],/usr/local/var,g' \
-e 's,[@]pkgdatadir[@],/usr/local/share/ovn,g' \
-e 's,[@]sysconfdir[@],/usr/local/etc,g' \
-e 's,[@]bindir[@],/usr/local/bin,g' \
-e 's,[@]sbindir[@],/usr/local/sbin,g' \
-e 
's,[@]abs_builddir[@],/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace,g'
 \
-e 
's,[@]abs_top_srcdir[@],/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace,g'
 \
  > rhel/ovn-fedora.spec.tmp
mv rhel/ovn-fedora.spec.tmp rhel/ovn-fedora.spec
touch -c manpage-check
./build-aux/cksum-schema-check ovn-nb.ovsschema ovn-nb.ovsschema.stamp
ovn-nb.ovsschema:4: The checksum "204590300 28863" was calculated from the 
schema file and does not match cksum field in the schema file - you should 
probably update the version number and the checksum in the schema file with the 
value listed here.
make[1]: *** [ovn-nb.ovsschema.stamp] Error 1
make[1]: Leaving directory 
`/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace'
make: *** [all] Error 2


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 v8 ovn] ovn-northd: introduce new allow-stateless ACL verb

[Ihar Hrachyshka]

For allow-stateless ACLs, bypass connection tracking by avoiding
setting ct hints for matching traffic. Avoid sending all traffic to ct
when a stateful ACL is present.

===

Reusing an existing 'allow' verb for stateless matching would have its
drawbacks, specifically, when it comes to backwards incompatibility of
the new behavior with existing environments. When using "allow" ACLs
in mixed allow/allow-related environment, we still commit "allow"
traffic to conntrack. This unnecessarily hits performance when mixed
ACL action types were used for the same datapath. This is why we
introduce a new action verb to describe stateless behavior.

Another complexity to consider is the fact that with stateless
matching, one would not be able to rely on 'related' magic that
guarantees that reply traffic is passed through. Instead, the user
would have to accurately define matching rules both for request and
reply directions of a protocol session. Specifically, when allowing
ICMP for a specific peer host, one has to define 'allow-stateless'
rules that would match against ip.dst for request direction and ip.src
for reply direction. Other protocols and scenarios will require their
own fine grained matching approaches implemented by the user.

===

For performance measurements, qperf was used. Tests were executed on two
setups:

1) ovn-fake-multinode virtual environment;
2) Supermicro SYS-5039MS-H8TRF 3-node RH-OSP 16.x cluster with 2 compute
   nodes, NIC: "Intel Corporation Ethernet Controller XXV710 for 25GbE
   SFP28 (rev 02)", using fake_nova_driver to avoid qemu overhead.

1) ovn-fake-multinode:

Performance measured between two virtual nodes, two ports
that belong to different LSs connected via router. Using qperf,
performance was measured for UDP, TCP, SCTP protocols (using
_lat and _bw tests). The qperf version used:
0.4.9-16.fc31.x86_64.  Each test scenario was executed five times and
averages compared.

Tests were executed with `allow-stateless` rules for the tested
protocol and `allow-related` for another protocol set for both ports,
both directions, e.g. for TCP scenario, the following ACLs were
defined:

ovn-nbctl acl-add sw0 to-lport 100 tcp allow-stateless
ovn-nbctl acl-add sw0 from-lport 100 tcp allow-stateless
ovn-nbctl acl-add sw1 to-lport 100 tcp allow-stateless
ovn-nbctl acl-add sw1 from-lport 100 tcp allow-stateless

ovn-nbctl acl-add sw0 to-lport 100 sctp allow-related
ovn-nbctl acl-add sw0 from-lport 100 sctp allow-related
ovn-nbctl acl-add sw1 to-lport 100 sctp allow-related
ovn-nbctl acl-add sw1 from-lport 100 sctp allow-related

In this particular environment, improvement was seen in send_bw,
latency, and msg_rate measurements, where applicable, for all three
protocols under test.

for UDP, send_bw: 293.6 MB/sec => 313.2 MB/sec (+6.68%)
 latency: 16 us => 14.08 us (-12%)
 msg_rate: 62.56 K/sec => 71.06 K/sec (+13.59%)

for TCP, latency: 18.6 us => 14.88 us (-20%)
 msg_rate: 53.8 K/sec => 67.28 K/sec (+25.06%)

for SCTP, latency: 21.98 us => 19.42 us (-11.65%)
  msg_rate: 45.58 K/sec => 51.54 K/sec (+13.08%)

Interestingly, some performance improvement was also seen for the same
scenarios with no ACLs set at all, albeit significantly more
negligible.

for UDP, send_bw: 320.0 MB/sec => 338.6 MB/sec (+5.81%)
 latency: 13.74 us => 12.88 us (-6.68%)
 msg_rate: 73.02 K/sec => 77.84 K/sec (+6.6%)

for TCP, latency: 15.62 us => 14.26 us (-9.54%)
 msg_rate: 64.02 K/sec => 70.26 K/sec (+9.75%)

for SCTP, latency: 19.56 us => 18.16 us (-7.16%)
  msg_rate: 51.16 K/sec => 55.12 K/sec (+7.74%)

2) Supermicro RH-OSP cluster:

Two scenarios executed:
- connectivity between two ports on the same switch
- connectivity between two ports on different switches connected via
  router

For the same switch, improvements are as follows:
- TCP latency: -5.3%, UDP latency: -13.5%, SCTP latency: -10.8%
- TCP bw: +0.8%, UDP bw, +12.25%, SCTP bw: +21.29%

For different switches, improvements are as follows:
- TCP latency: -6%, UDP: -11.2%, SCTP: -0.2%
- TCP bw: -0.7%, UDP bw: +2%, SCTP bw: +9.9%

The effect is more noticeable in same switch scenario.

Comparable numbers can be captured with iperf. It may be useful to run
more tests in a more elaborate (bare metal) environment.

===

The patch takes inspiration from a now abandoned patch:

"ovn-northd: Support mixing stateless/stateful ACLs with
Stateless_Filter." by Dumitru Ceara.

The original patch assumed CMS doesn't require full flexibility of
matching rules for stateless matching (for example, to be used by
OpenShift). But other CMS interfaces may require the same
customizability for stateless as well as stateful matching, like in
OpenStack Neutron API. Which is why this patch reuses existing ACL
object type to describe stateless rules.

Signed-off-by: Ihar Hrachyshka 

---

v1: initial version.
v2: rebased after conflict.
v3: added ddlog implementation.
v3: apply stateless skip-hint-set rules to 

Re: [ovs-dev] [PATCH ovn v6] ovn-controller: Split logical flow and phsyical flow processing.

[Han Zhou]

On Thu, Apr 29, 2021 at 8:10 AM Numan Siddique  wrote:
>
> On Mon, Apr 26, 2021 at 7:18 PM Han Zhou  wrote:
> >
> > On Mon, Apr 12, 2021 at 6:23 AM  wrote:
> > >
> > > From: Numan Siddique 
> > >
> > > Presently, the 'flow_output' engine node recomputes physical
> > > flows by calling physical_run() in the 'physical_flow_changes'
> > > handler in some scenarios.  Because of this, an engine run can
> > > do a full recompute of physical flows but not full recompute
> > > of logical flows.  Although this works now, it is problematic
> > > as the same desired flow table is used for both physical and
> > > logical flows.
> > >
> > > This patch now separates the handling of logical flows and
> > > physical flows and removes the 'physical_flow_changes' engine
> > > node.  Two separate engine nodes are added - lflow_output and
> > > pflow_output with their own flow tables and these two nodes are
> > > now inputs to the main engine node - flow_output.  This separation
> > > reflects the data dependency more clearly.
> > >
> > > CC: Han Zhou 
> > > Signed-off-by: Numan Siddique 
> > > ---
> > > v5 -> v6
> > > 
> > >   * Missed out checking in the uncommitted code in ofctrl.c in v4. v5
> > > fixes it.
> > >   * v4 accidently modified ovs submodule commit id. v5 reverts it.
> > >
> >
> > Thanks Numan for the revision. I think you meant "v6" fixes/reverts ...,
> > right?
>
> Thanks for the review comments.
>
> Seems like a typo from me. I will correct it in v7.
>
> > Anyway, please see some more comments below. The major comments are
> > regarding the noop handler usage for lflow_output and pflow_output.
Others
> > are very minor ones.
> >
> > Han
> >
> > > v4 -> v5
> > > -
> > >   * Addressed Han's comments.
> > >
> > > v3 -> v4
> > > -
> > >   * Addressed Mark G's comments.
> > >   * Rebased to resolve conflicts.
> > >
> > > v2 -> v3
> > > -
> > >   * Rebased to resolve conflicts.
> > >
> > > v1 -> v2
> > > -
> > >   * Rebased to resolve conflicts.
> > >
> > >  TODO.rst|   6 +
> > >  controller/ofctrl.c |  91 +++--
> > >  controller/ofctrl.h |   6 +-
> > >  controller/ovn-controller.c | 687
++--
> > >  4 files changed, 419 insertions(+), 371 deletions(-)
> > >
> > > diff --git a/TODO.rst b/TODO.rst
> > > index ecfe62870f..0a14b5219a 100644
> > > --- a/TODO.rst
> > > +++ b/TODO.rst
> > > @@ -166,3 +166,9 @@ OVN To-do List
> > >  to find a way of determining if routing has already been executed
> > (on a
> > >  different hypervisor) for the IP multicast packet being processed
> > locally
> > >  in the router pipeline.
> > > +
> > > +* ovn-controller Incremental processing
> > > +
> > > +  * physical.c has a global simap -localvif_to_ofport which stores
the
> > > +local OVS interfaces and the ofport numbers. Move this to the
engine
> > data
> > > +of the engine data node - ed_type_pflow_output.
> > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> > > index 415d9b7e16..346b791f78 100644
> > > --- a/controller/ofctrl.c
> > > +++ b/controller/ofctrl.c
> > > @@ -172,7 +172,7 @@ struct sb_flow_ref {
> > >  struct uuid sb_uuid;
> > >  };
> > >
> > > -/* A installed flow, in static variable installed_flows.
> > > +/* A installed flow, in static variable
> > installed_lflows/installed_pflows.
> > >   *
> > >   * Installed flows are updated in ofctrl_put for maintaining the flow
> > >   * installation to OVS. They are updated according to desired flows:
> > either by
> > > @@ -233,7 +233,7 @@ static struct desired_flow
> > *desired_flow_lookup_conjunctive(
> > >  static void desired_flow_destroy(struct desired_flow *);
> > >
> > >  static struct installed_flow *installed_flow_lookup(
> > > -const struct ovn_flow *target);
> > > +const struct ovn_flow *target, struct hmap *installed_flows);
> > >  static void installed_flow_destroy(struct installed_flow *);
> > >  static struct installed_flow *installed_flow_dup(struct desired_flow
*);
> > >  static struct desired_flow *installed_flow_get_active(struct
> > installed_flow *);
> > > @@ -301,9 +301,12 @@ static ovs_be32 xid, xid2;
> > >   * zero, to avoid unbounded buffering. */
> > >  static struct rconn_packet_counter *tx_counter;
> > >
> > > -/* Flow table of "struct ovn_flow"s, that holds the flow table
currently
> > > - * installed in the switch. */
> > > -static struct hmap installed_flows;
> > > +/* Flow table of "struct ovn_flow"s, that holds the logical flow
table
> > > + * currently installed in the switch. */
> > > +static struct hmap installed_lflows;
> > > +/* Flow table of "struct ovn_flow"s, that holds the physical flow
table
> > > + * currently installed in the switch. */
> > > +static struct hmap installed_pflows;
> > >
> > >  /* A reference to the group_table. */
> > >  static struct ovn_extend_table *groups;
> > > @@ -342,7 +345,8 @@ ofctrl_init(struct ovn_extend_table *group_table,
> > >  swconn = 

Re: [ovs-dev] [PATCH v1 0/8] RCU: Add blocking mode for debugging

[Ben Pfaff]

On Thu, May 06, 2021 at 12:37:36AM +0200, Gaëtan Rivet wrote:
> On Wed, May 5, 2021, at 21:36, Ben Pfaff wrote:
> > On Wed, Apr 28, 2021 at 01:03:24AM +0200, Gaetan Rivet wrote:
> > > This series adds a compilation option that changes the behavior of the RCU
> > > module. Once enabled, RCU reclamation by user threads becomes blocking 
> > > until
> > > the RCU threads has executed the scheduled callbacks.
> > 
> > It's a great idea to improve the quality of the tests, and RCU can be a
> > sticking point.
> > 
> > I don't quite understand the description.  I think that's for one
> > particular reason: I don't understand yet what it causes to block.
> > Would you mind giving an example?  I haven't yet read the patches (I
> > might not; I don't plan to review this series in detail), so maybe
> > there is an example in one of the patch.  If so, then please just point
> > me to that one.
> 
> Hello Ben,
> 
> The feature unit-test in patch 4 gives an example:
> https://patchwork.ozlabs.org/project/openvswitch/patch/6de208dfd9d2fdea5999140c88632d42bdfe428b.1619564350.git.gr...@u256.net/
> 
> In short:
> 
> char *x = xmalloc(1);
> x[0] = 'a';
> ovsrcu_postpone(free, x);
> ovsrcu_quiesce();
> x[0] = 'b'; /* potential UAF. */
> 
> If the RCU is made blocking, the user thread will wait at the end of 
> 'ovsrcu_quiesce()'
> for 'free(x)' to be executed. Only threads having scheduled callbacks would 
> wait.
> This resolves the above race condition. As it executes deterministically, if 
> ASAN is
> enabled as well it will detect the UAF.

This is a good example, thank you.  I think that it would be useful to
put this or a similar example into the documentation.

> In development, it could help verify that RCU is properly used.
> As it makes ASAN more effective, the RCU could be made blocking in 
> conjunction in CI jobs.
> 
> > This message is a good introduction to the series.  Usually,
> > introductory messages don't make it into the repository, so I hope that
> > this material is also included in commit messages or in documentation or
> > comments.
> 
> Most of the information here is explained in patch 8 commit log:
> https://patchwork.ozlabs.org/project/openvswitch/patch/54575393370fa6076ba6ce7201f36bbfecbe9039.1619564350.git.gr...@u256.net/
> 
> It also gives the configure option and how to test it.
> However, this feature is not otherwise documented.
> I could add a description in lib/ovs-rcu.h?

I think that would be a good place for it.  You could add a new
"Debugging" section at the end of the big comment, for example.

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

Re: [ovs-dev] [PATCH 0/7] OVSDB 2-Tier deployment.

[Ben Pfaff]

On Thu, May 06, 2021 at 12:34:00AM +0200, Ilya Maximets wrote:
> On 5/5/21 11:40 PM, Ben Pfaff wrote:
> > I think that this series has the details that one needs to understand it
> > at a high level, but I didn't see a high-level overview of its intended
> > use.  I think the idea is that, for a use case like OVN, one would
> > deploy a clustered OVS database, and then the second tier would be a
> > collection of replicas on top of that cluster.  If that's the case, I
> > think that adding a paragraph to an appropriate high-level documentation
> > file for OVSDB explaining it would be helpful, and possibly a (ASCII
> > art?) diagram.
> > 
> 
> Yes.  That is the intended use case.   And I agree that current series
> has only bare minimum of documentation.  I'll include the patch that
> adds a new paragraph to Documentation/topics/ovsdb-replication.rst to
> describe how to scale out read access (existing functionality that is
> documented with a single sentence in ovsdb(7) "A set of replicas that do  
> serve clients could be used to scale out read access to the primary
> database.") and how to use the same schema for an OVN deployment with
> transaction forwarding enabled (new features).  This could be done in
> v2 or as a separate patch.
> 
> I have a following ASCII art for the deployment schema:
> [...]

OK, great!

OVS and OVN are usually good at documenting details, but not the high
level.  It's a syndrome of starting out as a project that implemented a
standard (OpenFlow): we didn't document the high level because we
assumed everyone was already familiar with OpenFlow and what it was good
for at a high level.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v1 0/8] RCU: Add blocking mode for debugging

[Gaëtan Rivet]

On Wed, May 5, 2021, at 21:36, Ben Pfaff wrote:
> On Wed, Apr 28, 2021 at 01:03:24AM +0200, Gaetan Rivet wrote:
> > This series adds a compilation option that changes the behavior of the RCU
> > module. Once enabled, RCU reclamation by user threads becomes blocking until
> > the RCU threads has executed the scheduled callbacks.
> 
> It's a great idea to improve the quality of the tests, and RCU can be a
> sticking point.
> 
> I don't quite understand the description.  I think that's for one
> particular reason: I don't understand yet what it causes to block.
> Would you mind giving an example?  I haven't yet read the patches (I
> might not; I don't plan to review this series in detail), so maybe
> there is an example in one of the patch.  If so, then please just point
> me to that one.

Hello Ben,

The feature unit-test in patch 4 gives an example:
https://patchwork.ozlabs.org/project/openvswitch/patch/6de208dfd9d2fdea5999140c88632d42bdfe428b.1619564350.git.gr...@u256.net/

In short:

char *x = xmalloc(1);
x[0] = 'a';
ovsrcu_postpone(free, x);
ovsrcu_quiesce();
x[0] = 'b'; /* potential UAF. */

If the RCU is made blocking, the user thread will wait at the end of 
'ovsrcu_quiesce()'
for 'free(x)' to be executed. Only threads having scheduled callbacks would 
wait.
This resolves the above race condition. As it executes deterministically, if 
ASAN is
enabled as well it will detect the UAF.

In development, it could help verify that RCU is properly used.
As it makes ASAN more effective, the RCU could be made blocking in conjunction 
in CI jobs.

> 
> This message is a good introduction to the series.  Usually,
> introductory messages don't make it into the repository, so I hope that
> this material is also included in commit messages or in documentation or
> comments.

Most of the information here is explained in patch 8 commit log:
https://patchwork.ozlabs.org/project/openvswitch/patch/54575393370fa6076ba6ce7201f36bbfecbe9039.1619564350.git.gr...@u256.net/

It also gives the configure option and how to test it.
However, this feature is not otherwise documented.
I could add a description in lib/ovs-rcu.h?

> 
> Thanks,
> 
> Ben.
> 


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

Re: [ovs-dev] [PATCH 0/7] OVSDB 2-Tier deployment.

[Ilya Maximets]

On 5/5/21 11:40 PM, Ben Pfaff wrote:
> On Sat, May 01, 2021 at 02:55:41AM +0200, Ilya Maximets wrote:
>> Replication can be used to scale out read-only access to the database.
>> But there are clients that are not read-only, but read-mostly.
>> One of the main examples is ovn-controller that mostly monitors
>> updates from the Southbound DB, but needs to claim ports by sending
>> transactions that changes some database tables.
>>
>> Southbound database serves lots of connections: all connections
>> from ovn-controllers and some service connections from cloud
>> infrastructure, e.g. some OpenStack agents are monitoring updates.
>> At a high scale and with a big size of the database ovsdb-server
>> spends too much time processing monitor updates and it's required
>> to move this load somewhere else.  This patch-set aims to introduce
>> required functionality to scale out read-mostly connections by
>> replication.
>>
>> Replication mode natively supports replication of standalone and
>> clustered databases, so it will work for any type of OVN deployment.
> 
> I think that this series has the details that one needs to understand it
> at a high level, but I didn't see a high-level overview of its intended
> use.  I think the idea is that, for a use case like OVN, one would
> deploy a clustered OVS database, and then the second tier would be a
> collection of replicas on top of that cluster.  If that's the case, I
> think that adding a paragraph to an appropriate high-level documentation
> file for OVSDB explaining it would be helpful, and possibly a (ASCII
> art?) diagram.
> 

Yes.  That is the intended use case.   And I agree that current series
has only bare minimum of documentation.  I'll include the patch that
adds a new paragraph to Documentation/topics/ovsdb-replication.rst to
describe how to scale out read access (existing functionality that is
documented with a single sentence in ovsdb(7) "A set of replicas that do  
serve clients could be used to scale out read access to the primary
database.") and how to use the same schema for an OVN deployment with
transaction forwarding enabled (new features).  This could be done in
v2 or as a separate patch.

I have a following ASCII art for the deployment schema:


+-+
| RAFT CLUSTER|
|   +-+ ovsdb-server-1 +--+   |
|   | +   |   |
|   + |   +   |
| +--+ovsdb-server-2 +|--+ ovsdb-server-3+--+ |
| |   | | |
+-|---|-|-+
  |   | |
  |   +---++  +-+
  +   |   +|  | +
  +- ovsdb-replica-1 -+   2  ovsdb-replica-3   4 ... N-1  ovsdb-replica-N
  ||  |  |   | |   | | |
  ++  ++
client-1 client-2 client-3         client-M

For OVN setup: ovsdb-server-{1,2,3} - clustered Southbound DB
   ovsdb-replica-{1..N} - replication servers
   client-{1..M}- ovn-controller's

This schema also reflects the fact that replica doesn't replicate the
cluster, but a particular server in a cluster (see "missing part #2" in
the cover letter).  This also should be reflected in the high-level doc.

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

Re: [ovs-dev] [PATCH 0/7] OVSDB 2-Tier deployment.

[Ben Pfaff]

On Sat, May 01, 2021 at 02:55:41AM +0200, Ilya Maximets wrote:
> Replication can be used to scale out read-only access to the database.
> But there are clients that are not read-only, but read-mostly.
> One of the main examples is ovn-controller that mostly monitors
> updates from the Southbound DB, but needs to claim ports by sending
> transactions that changes some database tables.
> 
> Southbound database serves lots of connections: all connections
> from ovn-controllers and some service connections from cloud
> infrastructure, e.g. some OpenStack agents are monitoring updates.
> At a high scale and with a big size of the database ovsdb-server
> spends too much time processing monitor updates and it's required
> to move this load somewhere else.  This patch-set aims to introduce
> required functionality to scale out read-mostly connections by
> replication.
> 
> Replication mode natively supports replication of standalone and
> clustered databases, so it will work for any type of OVN deployment.

I think that this series has the details that one needs to understand it
at a high level, but I didn't see a high-level overview of its intended
use.  I think the idea is that, for a use case like OVN, one would
deploy a clustered OVS database, and then the second tier would be a
collection of replicas on top of that cluster.  If that's the case, I
think that adding a paragraph to an appropriate high-level documentation
file for OVSDB explaining it would be helpful, and possibly a (ASCII
art?) diagram.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] flow: Read recirculation depth once for whole batch in miniflow_extract

[Paolo Valerio]

Balazs Nemeth  writes:

> The call to recirc_depth_get involves accessing a TLS value which is
> slower than accessing the stack.
>
> Signed-off-by: Balazs Nemeth 
> ---

LGTM,

Acked-by: Paolo Valerio 

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

Re: [ovs-dev] [PATCH] lib: Introduce netlink-devlink library

[0-day Robot]

Bleep bloop.  Greetings Frode Nordahl, 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:
ERROR: Inappropriate spacing in pointer declaration
WARNING: Line lacks whitespace around operator
#874 FILE: lib/netlink-devlink.c:139:
(bool (*)(struct ofpbuf*, void*))_dl_parse_port_policy,

ERROR: Inappropriate spacing in pointer declaration
ERROR: Inappropriate spacing around cast
WARNING: Line lacks whitespace around operator
#875 FILE: lib/netlink-devlink.c:140:
(void*)port_entry);

ERROR: Inappropriate spacing in pointer declaration
WARNING: Line lacks whitespace around operator
#884 FILE: lib/netlink-devlink.c:149:
(bool (*)(struct ofpbuf*, void*))_dl_parse_info_policy,

ERROR: Inappropriate spacing in pointer declaration
ERROR: Inappropriate spacing around cast
WARNING: Line lacks whitespace around operator
#885 FILE: lib/netlink-devlink.c:150:
(void*)info_entry);

ERROR: Improper whitespace around control block
#901 FILE: lib/netlink-devlink.c:166:
switch(policy[attr_idx].type) {

ERROR: Inappropriate bracing around statement
#1068 FILE: lib/netlink-devlink.c:333:
else

WARNING: Line lacks whitespace around operator
#1206 FILE: lib/netlink-devlink.c:471:
||!attr_fill_version(DEVLINK_ATTR_INFO_VERSION_RUNNING, attrs,

WARNING: Line lacks whitespace around operator
#1208 FILE: lib/netlink-devlink.c:473:
||!attr_fill_version(DEVLINK_ATTR_INFO_VERSION_STORED, attrs,

WARNING: Line is 95 characters long (recommended limit is 79)
#1710 FILE: utilities/devlink.c:81:
VLOG_INFO("function eth_addr: "ETH_ADDR_FMT, 
ETH_ADDR_ARGS(port_entry->function.eth_addr));

ERROR: Improper whitespace around control block
#1826 FILE: utilities/devlink.c:197:
if(!nl_dl_parse_port_policy(, _entry)) {

WARNING: Line is 80 characters long (recommended limit is 79)
#1831 FILE: utilities/devlink.c:202:
genl->cmd == DEVLINK_CMD_PORT_GET ? "DEVLINK_CMD_PORT_GET" :

WARNING: Line is 80 characters long (recommended limit is 79)
#1832 FILE: utilities/devlink.c:203:
genl->cmd == DEVLINK_CMD_PORT_SET ? "DEVLINK_CMD_PORT_SET" :

WARNING: Line is 80 characters long (recommended limit is 79)
#1833 FILE: utilities/devlink.c:204:
genl->cmd == DEVLINK_CMD_PORT_NEW ? "DEVLINK_CMD_PORT_NEW" :

WARNING: Line is 80 characters long (recommended limit is 79)
#1834 FILE: utilities/devlink.c:205:
genl->cmd == DEVLINK_CMD_PORT_DEL ? "DEVLINK_CMD_PORT_DEL" :

Lines checked: 1877, Warnings: 11, Errors: 9


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] lib: Introduce netlink-devlink library

[Frode Nordahl]

The devlink interface was introduced [0] in the Linux 4.6 time
frame and has since gained traction among multiple hardware
vendors.

The devlink-port [1] and devlink-info[2] interfaces are
particularly useful for managing NICs connected to multiple
distinct CPUs such as SmartNICs.

In such a topology it would be useful to be able to offload
Open vSwitch and OVN onto the NIC SoC operating system and this
library will help with discovering and managing ports representing
resources made available on the host from the NIC SoC side.

The library will be consumed by upcoming proposed changes to OVN,
and I think it makes sense to maintain it together with the Open
vSwitch netlink library code. The proposed changes are also
referenced in specifications proposed to the OpenStack project [3][4].

0: 
https://lore.kernel.org/netdev/1456504351-18871-1-git-send-email-j...@resnulli.us/
1: 
https://github.com/torvalds/linux/blob/master/Documentation/networking/devlink/devlink-port.rst
2: 
https://github.com/torvalds/linux/blob/master/Documentation/networking/devlink/devlink-info.rst
3: https://review.opendev.org/c/openstack/nova-specs/+/787458
4: https://review.opendev.org/c/openstack/neutron-specs/+/788821
Signed-off-by: Frode Nordahl 
---
 include/linux/automake.mk |   1 +
 include/linux/devlink.h   | 625 ++
 include/openvswitch/types.h   |   8 +
 lib/automake.mk   |   2 +
 lib/netlink-devlink.c | 498 
 lib/netlink-devlink.h | 114 ++
 lib/netlink.c |  16 +
 lib/netlink.h |   5 +
 lib/packets.h |   1 +
 tests/.gitignore  |   3 +
 tests/automake.mk |  16 +
 tests/system-devlink-info.at  |   9 +
 tests/system-devlink-port.at  |  12 +
 tests/system-devlink-testsuite.at |  45 +++
 utilities/.gitignore  |   1 +
 utilities/automake.mk |   3 +
 utilities/devlink.c   | 245 
 17 files changed, 1604 insertions(+)
 create mode 100644 include/linux/devlink.h
 create mode 100644 lib/netlink-devlink.c
 create mode 100644 lib/netlink-devlink.h
 create mode 100644 tests/system-devlink-info.at
 create mode 100644 tests/system-devlink-port.at
 create mode 100644 tests/system-devlink-testsuite.at
 create mode 100644 utilities/devlink.c

diff --git a/include/linux/automake.mk b/include/linux/automake.mk
index 8f063f482..8718f980d 100644
--- a/include/linux/automake.mk
+++ b/include/linux/automake.mk
@@ -1,4 +1,5 @@
 noinst_HEADERS += \
+   include/linux/devlink.h \
include/linux/netlink.h \
include/linux/netfilter/nf_conntrack_sctp.h \
include/linux/pkt_cls.h \
diff --git a/include/linux/devlink.h b/include/linux/devlink.h
new file mode 100644
index 0..28ea92b62
--- /dev/null
+++ b/include/linux/devlink.h
@@ -0,0 +1,625 @@
+/*
+ * The kernel devlink interface has gained a number of additions in recent
+ * kernel versions. To allow Open vSwitch to consume these interfaces in its
+ * runtime environment regardless of what kernel version was available at build
+ * time, and also avoiding an elaborate set of autoconf macros to check for
+ * presence of individual pieces, we include the entire file here.
+ *
+ * Source:
+ * 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/include/uapi/linux/devlink.h
 @ a556dded9c23c51c82654f1ebe389cbc0bc22057 */
+#if !defined(__KERNEL__)
+#ifndef __UAPI_LINUX_DEVLINK_WRAPPER_H
+#define __UAPI_LINUX_DEVLINK_WRAPPER_H 1
+/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
+/*
+ * include/uapi/linux/devlink.h - Network physical device Netlink interface
+ * Copyright (c) 2016 Mellanox Technologies. All rights reserved.
+ * Copyright (c) 2016 Jiri Pirko 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef _UAPI_LINUX_DEVLINK_H_
+#define _UAPI_LINUX_DEVLINK_H_
+
+#include 
+
+#define DEVLINK_GENL_NAME "devlink"
+#define DEVLINK_GENL_VERSION 0x1
+#define DEVLINK_GENL_MCGRP_CONFIG_NAME "config"
+
+enum devlink_command {
+   /* don't change the order or add anything between, this is ABI! */
+   DEVLINK_CMD_UNSPEC,
+
+   DEVLINK_CMD_GET,/* can dump */
+   DEVLINK_CMD_SET,
+   DEVLINK_CMD_NEW,
+   DEVLINK_CMD_DEL,
+
+   DEVLINK_CMD_PORT_GET,   /* can dump */
+   DEVLINK_CMD_PORT_SET,
+   DEVLINK_CMD_PORT_NEW,
+   DEVLINK_CMD_PORT_DEL,
+
+   DEVLINK_CMD_PORT_SPLIT,
+   DEVLINK_CMD_PORT_UNSPLIT,
+
+   DEVLINK_CMD_SB_GET, /* can dump */
+   DEVLINK_CMD_SB_SET,
+   DEVLINK_CMD_SB_NEW,
+   DEVLINK_CMD_SB_DEL,
+
+   DEVLINK_CMD_SB_POOL_GET,/* can dump */
+   

Re: [ovs-dev] [PATCH v1 0/8] RCU: Add blocking mode for debugging

[Ben Pfaff]

On Wed, Apr 28, 2021 at 01:03:24AM +0200, Gaetan Rivet wrote:
> This series adds a compilation option that changes the behavior of the RCU
> module. Once enabled, RCU reclamation by user threads becomes blocking until
> the RCU threads has executed the scheduled callbacks.

It's a great idea to improve the quality of the tests, and RCU can be a
sticking point.

I don't quite understand the description.  I think that's for one
particular reason: I don't understand yet what it causes to block.
Would you mind giving an example?  I haven't yet read the patches (I
might not; I don't plan to review this series in detail), so maybe
there is an example in one of the patch.  If so, then please just point
me to that one.

This message is a good introduction to the series.  Usually,
introductory messages don't make it into the repository, so I hope that
this material is also included in commit messages or in documentation or
comments.

Thanks,

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

Re: [ovs-dev] [PATCH V6 00/13] Netdev vxlan-decap offload

[Gaëtan Rivet]

On Wed, Apr 21, 2021, at 10:20, Kovacevic, Marko wrote:
> > On 4/7/2021 12:10 PM, Sriharsha Basavapatna wrote:
> > >
> > > On Sun, Apr 4, 2021 at 3:25 PM Eli Britstein  > > > wrote:
> > >
> > > VXLAN decap in OVS-DPDK configuration consists of two flows:
> > > F1: in_port(ens1f0),eth(),ipv4(),udp(),
> > > actions:tnl_pop(vxlan_sys_4789)
> > > F2: tunnel(),in_port(vxlan_sys_4789),eth(),ipv4(), actions:ens1f0_0
> > >
> > > F1 is a classification flow. It has outer headers matches and it
> > > classifies the packet as a VXLAN packet, and using tnl_pop action the
> > > packet continues processing in F2.
> > > F2 is a flow that has matches on tunnel metadata as well as on the
> > > inner
> > > packet headers (as any other flow).
> > >
> > > In order to fully offload VXLAN decap path, both F1 and F2 should be
> > > offloaded. As there are more than one flow in HW, it is possible that
> > > F1 is done by HW but F2 is not. Packet is received by SW, and
> > > should be
> > > processed starting from F2 as F1 was already done by HW.
> > > Rte_flows are applicable only on physical port IDs. Keeping the
> > > original
> > > physical in port on which the packet is received on enables applying
> > > vport flows (e.g. F2) on that physical port.
> > >
> > > This patch-set makes use of [1] introduced in DPDK 20.11, that
> > > adds API
> > > for tunnel offloads.
> > >
> > > Note that MLX5 PMD has a bug that the tnl_pop private actions must be
> > > first. In OVS it is not.
> > > Fixing this issue is scheduled for 21.05 (and stable 20.11.2).
> > > Meanwhile, tests were done with a workaround for it [2].
> > >
> > > v2-v1:
> > > - Tracking original in_port, and applying vport on that physical
> > > port instead of all PFs.
> > > v3-v2:
> > > - Traversing ports using a new API instead of flow_dump.
> > > - Refactor packet state recover logic, with bug fix for error
> > > pop_header.
> > > - One ref count for netdev in non-tunnel case.
> > > - Rename variables, comments, rebase.
> > > v4-v3:
> > > - Extract orig_in_port from physdev for flow modify.
> > > - Miss handling fixes.
> > > v5-v4:
> > > - Drop refactor offload rule creation commit.
> > > - Comment about setting in_port in restore.
> > > - Refactor vports flow offload commit.
> > > v6-v5:
> > > - Fixed duplicate netdev ref bug.
> > >
> > >
> > > Can you provide some info on this bug ?  and what changes were done to
> > > fix this ?
> > 
> > With v5, the 2 netdevs sent to ufid_to_rte_flow_associate are always
> > non-NULL (was not like this previously), and there was this line:
> > 
> > +    data->netdev = vport ? netdev_ref(vport) : physdev;
> > 
> > As the "vport" was always non-null, even for non-tunnels, it took
> > another ref of it, but in disassociate, only one close was done.
> > 
> > With v6 it is like this (changed arguments names a bit)
> > 
> > +    data->physdev = netdev != physdev ? netdev_ref(physdev) : physdev;
> > 
> > Checking the netdevs are different, not non-NULL.
> > 
> > > Thanks,
> > > -Harsha
> > >
> > >
> > > Travis:
> > > v1: https://travis-ci.org/github/elibritstein/OVS/builds/756418552
> > > v2
> > > :
> > > https://travis-ci.org/github/elibritstein/OVS/builds/758382963
> > > v3
> > > :
> > > https://travis-ci.org/github/elibritstein/OVS/builds/761089087
> > > v4
> > > :
> > > https://travis-ci.org/github/elibritstein/OVS/builds/763146966
> > > v5
> > > :
> > > https://travis-ci.org/github/elibritstein/OVS/builds/765271879
> > > v6
> > > :
> > > https://travis-ci.org/github/elibritstein/OVS/builds/765816800
> > > 
> > >
> > > GitHub Actions:
> > > v1: https://github.com/elibritstein/OVS/actions/runs/515334647
> > > 
> > > v2: https://github.com/elibritstein/OVS/actions/runs/554986007
> > > 
> > > v3: https://github.com/elibritstein/OVS/actions/runs/613226225
> > > 
> > > v4: https://github.com/elibritstein/OVS/actions/runs/658415274
> > > 
> > > v5: https://github.com/elibritstein/OVS/actions/runs/704357369
> > > 
> > > v6: 

Re: [ovs-dev] [PATCH ovn v7 4/5] pinctrl: Add Chassis MAC_Bindings for all router addresses.

[0-day Robot]

Bleep bloop.  Greetings Mark Michelson, 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 84 characters long (recommended limit is 79)
#368 FILE: controller/pinctrl.c:5672:
hmap_insert(chassis_mac_bindings, _binding->hmap_node, 
ch_binding->hash);

WARNING: Line is 82 characters long (recommended limit is 79)
#378 FILE: controller/pinctrl.c:5682:
struct ovsdb_idl_index 
*sbrec_mac_binding_by_lport_ip,

WARNING: Line is 83 characters long (recommended limit is 79)
#416 FILE: controller/pinctrl.c:5720:
 struct ovsdb_idl_index 
*sbrec_mac_binding_by_lport_ip,

WARNING: Line is 86 characters long (recommended limit is 79)
#495 FILE: controller/pinctrl.c:5810:
install_chassis_mac_bindings(ovnsb_idl_txn, 
sbrec_mac_binding_by_lport_ip,

WARNING: Line is 86 characters long (recommended limit is 79)
#509 FILE: controller/pinctrl.c:5824:
install_chassis_mac_bindings(ovnsb_idl_txn, 
sbrec_mac_binding_by_lport_ip,

WARNING: Line is 80 characters long (recommended limit is 79)
#603 FILE: ovn-sb.ovsschema:244:
  "refTable": "Datapath_Binding"}}},

Lines checked: 838, Warnings: 6, Errors: 0


build:
-e 's,[@]OVN_RUNDIR[@],/usr/local/var/run/ovn,g' \
-e 
's,[@]OVSBUILDDIR[@],/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR,g'
 \
-e 's,[@]VERSION[@],21.03.90,g' \
-e 's,[@]OVSVERSION[@],2.15.90,g' \
-e 's,[@]localstatedir[@],/usr/local/var,g' \
-e 's,[@]pkgdatadir[@],/usr/local/share/ovn,g' \
-e 's,[@]sysconfdir[@],/usr/local/etc,g' \
-e 's,[@]bindir[@],/usr/local/bin,g' \
-e 's,[@]sbindir[@],/usr/local/sbin,g' \
-e 
's,[@]abs_builddir[@],/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace,g'
 \
-e 
's,[@]abs_top_srcdir[@],/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace,g'
 \
  > utilities/ovn-lib.tmp
mv utilities/ovn-lib.tmp utilities/ovn-lib
/bin/sh 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/build-aux/missing
 autom4te --language=autotest -I '.' -o tests/testsuite.tmp tests/testsuite.at
mv tests/testsuite.tmp tests/testsuite
/bin/sh 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/build-aux/missing
 autom4te --language=autotest -I '.' -o tests/system-kmod-testsuite.tmp 
tests/system-kmod-testsuite.at
mv tests/system-kmod-testsuite.tmp tests/system-kmod-testsuite
/bin/sh 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/build-aux/missing
 autom4te --language=autotest -I '.' -o tests/system-userspace-testsuite.tmp 
tests/system-userspace-testsuite.at
mv tests/system-userspace-testsuite.tmp tests/system-userspace-testsuite
tests/ofproto-macros.at
See above for files that use tabs for indentation.
Please use spaces instead.
make[1]: *** [check-tabs] Error 1
make[1]: Leaving directory 
`/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace'
make: *** [all] Error 2


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 ovn v7 3/5] northd: Save all router addresses in Port_Bindings

[0-day Robot]

Bleep bloop.  Greetings Mark Michelson, 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 83 characters long (recommended limit is 79)
#252 FILE: northd/ovn-northd.c:3207:
ds_put_format(_networks, "%s", 
op->peer->lrp_networks.ea_s);

WARNING: Line is 83 characters long (recommended limit is 79)
#264 FILE: northd/ovn-northd.c:3214:
ds_put_format(_networks, " 
is_chassis_resident(%s)",

WARNING: Line is 83 characters long (recommended limit is 79)
#270 FILE: northd/ovn-northd.c:3220:
ds_cstr(_networks), 
peer_is_gateway,

Lines checked: 759, Warnings: 3, 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 ovn v7 5/5] northd: Flood ARPs to routers for "unreachable" addresses.

[Mark Michelson]

Previously, ARP TPAs were filtered down only to "reachable" addresses.
Reachable addresses are all router interface addresses, as well as NAT
external addresses and load balancer VIPs that are within the subnet
handled by a router's port.

However, it is possible that in some configurations, CMSes purposely
configure NAT or load balancer addresses on a router that are outside
the router's subnets, and they expect the router to respond to ARPs for
those addresses.

This commit adds a higher priority flow to logical switches that makes
it so ARPs targeted at "unreachable" addresses are flooded to all ports.
This way, the ARPs can reach the router appropriately and receive a
response.

Reported at: https://bugzilla.redhat.com/show_bug.cgi?id=1929901

Signed-off-by: Mark Michelson 
---
 northd/ovn-northd.8.xml |   8 ++
 northd/ovn-northd.c | 158 +++-
 northd/ovn_northd.dl| 102 --
 tests/ovn-northd.at |  99 +
 tests/system-ovn.at | 107 +++
 5 files changed, 402 insertions(+), 72 deletions(-)

diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 54e88d3fa..bdfdd0ede 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -1535,6 +1535,14 @@ output;
 logical ports.
   
 
+  
+Priority-80 flows for each IP address/VIP/NAT address configured
+outside its owning router port's subnet. These flows match ARP
+requests and ND packets for the specific IP addresses.  Matched packets
+are forwarded only to the MC_FLOOD multicast group which
+contains all connected logical ports.
+  
+
   
 Priority-75 flows for each port connected to a logical router
 matching self originated ARP request/ND packets.  These packets
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 3a4d5323f..2b84dcd33 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -6477,44 +6477,51 @@ build_lswitch_rport_arp_req_self_orig_flow(struct 
ovn_port *op,
 ds_destroy();
 }
 
-/*
- * Ingress table 19: Flows that forward ARP/ND requests only to the routers
- * that own the addresses. Other ARP/ND packets are still flooded in the
- * switching domain as regular broadcast.
- */
 static void
-build_lswitch_rport_arp_req_flow_for_ip(struct sset *ips,
-int addr_family,
-struct ovn_port *patch_op,
-struct ovn_datapath *od,
-uint32_t priority,
-struct hmap *lflows,
-const struct ovsdb_idl_row *stage_hint)
+arp_nd_ns_match(struct sset *ips, int addr_family, struct ds *match)
 {
-struct ds match   = DS_EMPTY_INITIALIZER;
-struct ds actions = DS_EMPTY_INITIALIZER;
 
 /* Packets received from VXLAN tunnels have already been through the
  * router pipeline so we should skip them. Normally this is done by the
  * multicast_group implementation (VXLAN packets skip table 32 which
  * delivers to patch ports) but we're bypassing multicast_groups.
  */
-ds_put_cstr(, FLAGBIT_NOT_VXLAN " && ");
+ds_put_cstr(match, FLAGBIT_NOT_VXLAN " && ");
 
 if (addr_family == AF_INET) {
-ds_put_cstr(, "arp.op == 1 && arp.tpa == { ");
+ds_put_cstr(match, "arp.op == 1 && arp.tpa == {");
 } else {
-ds_put_cstr(, "nd_ns && nd.target == { ");
+ds_put_cstr(match, "nd_ns && nd.target == {");
 }
 
 const char *ip_address;
 SSET_FOR_EACH (ip_address, ips) {
-ds_put_format(, "%s, ", ip_address);
+ds_put_format(match, "%s, ", ip_address);
 }
 
-ds_chomp(, ' ');
-ds_chomp(, ',');
-ds_put_cstr(, "}");
+ds_chomp(match, ' ');
+ds_chomp(match, ',');
+ds_put_cstr(match, "}");
+}
+
+/*
+ * Ingress table 19: Flows that forward ARP/ND requests only to the routers
+ * that own the addresses. Other ARP/ND packets are still flooded in the
+ * switching domain as regular broadcast.
+ */
+static void
+build_lswitch_rport_arp_req_flow_for_reachable_ip(struct sset *ips,
+  int addr_family,
+  struct ovn_port *patch_op,
+  struct ovn_datapath *od,
+  uint32_t priority,
+  struct hmap *lflows,
+  const struct ovsdb_idl_row 
*stage_hint)
+{
+struct ds match   = DS_EMPTY_INITIALIZER;
+struct ds actions = DS_EMPTY_INITIALIZER;
+
+arp_nd_ns_match(ips, addr_family, );
 
 /* Send a the packet to the router pipeline.  If the switch has non-router
  * ports then flood it there as well.
@@ -6537,6 +6544,32 

[ovs-dev] [PATCH ovn v7 3/5] northd: Save all router addresses in Port_Bindings

[Mark Michelson]

northd currently stores logical switch port's configured "nat-addresses"
in the southbound Port_Binding "nat_addresses" column. This allows for
explicit configuration of which NAT addresses should be broadcast in
gARP messages by OVN.

This adds a similar column, "router_addresses" to the Port_Binding. The
difference is that this column contains all NAT, load balancer, and
router interface addresses, no matter the northbound configuration.

This column will be used in an upcoming commit.

Signed-off-by: Mark Michelson 
---
 northd/ovn-northd.c  | 217 +
 northd/ovn_northd.dl |  45 ++--
 ovn-sb.ovsschema |   5 +-
 ovn-sb.xml   |  20 
 tests/ovn-northd.at  | 253 +++
 5 files changed, 461 insertions(+), 79 deletions(-)

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index d8ee65a5f..38de13090 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -2486,7 +2486,7 @@ get_nat_addresses(const struct ovn_port *op, size_t *n)
 {
 size_t n_nats = 0;
 struct eth_addr mac;
-if (!op->nbrp || !op->od || !op->od->nbr
+if (!op || !op->nbrp || !op->od || !op->od->nbr
 || (!op->od->nbr->n_nat && !op->od->nbr->n_load_balancer)
 || !eth_addr_from_string(op->nbrp->mac, )) {
 *n = n_nats;
@@ -2878,6 +2878,123 @@ ovn_update_ipv6_prefix(struct hmap *ports)
 }
 }
 
+static void
+ovn_port_set_garp_addresses(const struct ovn_port *op, const char **nats,
+size_t n_nats, const char *router_networks,
+bool peer_is_gateway, bool peer_has_dist_gw_port)
+{
+const char *nat_addresses = smap_get(>nbsp->options,
+ "nat-addresses");
+size_t n_sources = 0;
+const char **source = NULL;
+if (nat_addresses && (peer_is_gateway || peer_has_dist_gw_port)) {
+if (!strcmp(nat_addresses, "router")) {
+/* We size this to n_nats + 1 since it can at most include
+ * all NAT addresses plus the router's networks.
+ */
+source = nats;
+n_sources = n_nats;
+/* Only accept manual specification of ethernet address
+ * followed by IPv4 addresses on type "l3gateway" ports. */
+} else if (peer_is_gateway) {
+struct lport_addresses laddrs;
+if (!extract_lsp_addresses(nat_addresses, )) {
+static struct vlog_rate_limit rl =
+VLOG_RATE_LIMIT_INIT(1, 1);
+VLOG_WARN_RL(, "Error extracting nat-addresses.");
+} else {
+destroy_lport_addresses();
+/* Allocate enough space for the nat_addresses plus
+ * the router networks
+ */
+source = _addresses;
+n_sources = 1;
+}
+}
+}
+
+size_t n_garps = 0;
+/* The "garps" array contains the addresses for which
+ * ovn-controller should send GARPs. This is controlled
+ * by the "nat-addresses" option on the logical switch.
+ *
+ * Size the garps array to one more than the number of NAT
+ * sources so that we can fit the NATs and the router
+ * networks.
+ */
+char **garps = xcalloc(n_sources + 1, sizeof *garps);
+for (n_garps = 0; n_garps < n_sources; n_garps++) {
+garps[n_garps] = xstrdup(source[n_garps]);
+}
+
+/* Add the router mac and IPv4 addresses to
+ * Port_Binding.nat_addresses so that GARP is sent for these
+ * IPs by the ovn-controller on which the distributed gateway
+ * router port resides if:
+ *
+ * -  op->peer has 'reside-on-redirect-chassis' set and the
+ *the logical router datapath has distributed router port.
+ *
+ * -  op->peer is distributed gateway router port.
+ *
+ * -  op->peer's router is a gateway router and op has a localnet
+ *port.
+ *
+ * Note: Port_Binding.nat_addresses column is also used for
+ * sending the GARPs for the router port IPs.
+ * */
+bool add_router_port_garp = false;
+if (peer_has_dist_gw_port &&
+(smap_get_bool(>peer->nbrp->options,
+  "reside-on-redirect-chassis", false) ||
+op->peer == op->peer->od->l3dgw_port)) {
+add_router_port_garp = true;
+} else if (peer_is_gateway && op->od->n_localnet_ports) {
+add_router_port_garp = true;
+}
+
+if (add_router_port_garp) {
+/* "garps" was allocated with enough space to hold
+ * this address without reallocating.
+ */
+garps[n_garps] = xstrdup(router_networks);
+n_garps++;
+}
+
+sbrec_port_binding_set_nat_addresses(op->sb,
+ (const char **) garps,
+ n_garps);
+for (size_t i = 0; i < n_garps; i++) {
+free(garps[i]);
+}
+free(garps);
+}
+
+static 

[ovs-dev] [PATCH ovn v7 4/5] pinctrl: Add Chassis MAC_Bindings for all router addresses.

[Mark Michelson]

Previous behavior of ovn-controller was to only create MAC bindings for
the same addresses for which we would send gARPs. That is:

* A logical switch has its "nat_addresses" configured.
* That logical switch has a localnet port.

This makes sense for gARPs: we only want to send gARPs for addresses
accessible from outside of OVN, and we only want to send gARPs for
addresses explicitly enabled by the administrator.

For directly adding MAC bindings, though, this makes less sense. In the
case where a VM or pod on the underlay network wishes to reach another
VM or pod via a DNAT or load balancer address, an ARP was required to
learn the MAC binding. When the information is all available in the
database, this seems like an unnecessary step.

This commit makes the following changes:
* MAC_Binding now has a "chassis_name" column. This is used to identify
  the chassis that added the MAC Binding IF the MAC Binding was auto-
  generated by ovn-controller. This is also used to ensure that stale
  bindings can be cleaned up.
* The term "local garp" has been replaced with "chassis MAC binding".
  Removing the word "garp" helps to remove any implication of packets
  being transmitted. The word "chassis" precedes "MAC binding" to
  indicate it is a MAC binding added by a chassis and with a
  chassis_name in its records. MAC bindings with no chassis_name were
  added dynamically due to reception of ARP/gARP packets.
* Chassis MAC Binding code has been separated from gARP code since they
  do not operate using the same sets of addresses.
* Chassis MAC Bindings are added for all router_addresses on a port
  binding. This means all NAT, Load Balancer, and router interface
  addresses are added for routers that are residents of the chassis.

Signed-off-by: Mark Michelson 
---
 controller/ovn-controller.c |   4 +
 controller/pinctrl.c| 300 +++-
 controller/pinctrl.h|   1 +
 northd/ovn-northd.c |   2 +-
 northd/ovn_northd.dl|   5 +-
 ovn-sb.ovsschema|   5 +-
 ovn-sb.xml  |   7 +
 tests/ofproto-macros.at |   5 +
 tests/ovn-controller.at | 179 +
 9 files changed, 433 insertions(+), 75 deletions(-)

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 6106a9661..86f5a3f9d 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -2480,6 +2480,9 @@ main(int argc, char *argv[])
 = ovsdb_idl_index_create2(ovnsb_idl_loop.idl,
   _fdb_col_mac,
   _fdb_col_dp_key);
+struct ovsdb_idl_index *sbrec_mac_binding_by_chassis
+= ovsdb_idl_index_create1(ovnsb_idl_loop.idl,
+  _mac_binding_col_chassis_name);
 
 ovsdb_idl_track_add_all(ovnsb_idl_loop.idl);
 ovsdb_idl_omit_alert(ovnsb_idl_loop.idl,
@@ -2904,6 +2907,7 @@ main(int argc, char *argv[])
 sbrec_port_binding_by_key,
 sbrec_port_binding_by_name,
 sbrec_mac_binding_by_lport_ip,
+sbrec_mac_binding_by_chassis,
 sbrec_igmp_group,
 sbrec_ip_multicast,
 sbrec_fdb_by_dp_key_mac,
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 523a45b9a..6a4199b8d 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -205,6 +205,7 @@ static void send_garp_rarp_prepare(
 struct ovsdb_idl_index *sbrec_port_binding_by_datapath,
 struct ovsdb_idl_index *sbrec_port_binding_by_name,
 struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
+struct ovsdb_idl_index *sbrec_mac_binding_by_chassis,
 const struct ovsrec_bridge *,
 const struct sbrec_chassis *,
 const struct hmap *local_datapaths,
@@ -3318,6 +3319,7 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
 struct ovsdb_idl_index *sbrec_port_binding_by_key,
 struct ovsdb_idl_index *sbrec_port_binding_by_name,
 struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
+struct ovsdb_idl_index *sbrec_mac_binding_by_chassis,
 struct ovsdb_idl_index *sbrec_igmp_groups,
 struct ovsdb_idl_index *sbrec_ip_multicast_opts,
 struct ovsdb_idl_index *sbrec_fdb_by_dp_key_mac,
@@ -3339,7 +3341,8 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
sbrec_port_binding_by_key, chassis);
 send_garp_rarp_prepare(ovnsb_idl_txn, sbrec_port_binding_by_datapath,
sbrec_port_binding_by_name,
-   sbrec_mac_binding_by_lport_ip, br_int, chassis,
+   sbrec_mac_binding_by_lport_ip,
+   sbrec_mac_binding_by_chassis, br_int, chassis,
local_datapaths, active_tunnels);
 

[ovs-dev] [PATCH ovn v7 1/5] northd: Swap src and dst eth addresses in router egress loop.

[Mark Michelson]

When a hairpin scenario is detected (i.e. egressing on gateway router
port to a NAT external address), we loop back to the ingress router
pipeline and swap the inport and outport. This is intended to allow for
the traffic to get DNATted. In practice, though, the ethernet
destination has not been updated to be the distributed gateway port's
address, and so the packet is immediately dropped.

This fixes the issue by swapping source and dest eth addressses when
doing the hairpin redirection. This way, the packet appears to be
arriving on the gateway port, and it gets handled as expected.

Reported at: https://bugzilla.redhat.com/show_bug.cgi?id=1929901

Signed-off-by: Mark Michelson 
---
 northd/ovn-northd.c  |   1 +
 northd/ovn_northd.dl |   1 +
 tests/system-ovn.at  | 111 +++
 3 files changed, 113 insertions(+)

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 94fae5648..d8ee65a5f 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -11734,6 +11734,7 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od,
 ds_put_format(actions,
   "clone { ct_clear; "
   "inport = outport; outport = \"\"; "
+  "eth.dst <-> eth.src; "
   "flags = 0; flags.loopback = 1; ");
 for (int j = 0; j < MFF_N_LOG_REGS; j++) {
 ds_put_format(actions, "reg%d = 0; ", j);
diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 7953325aa..c29cf1379 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -5901,6 +5901,7 @@ for (r in (.lr = lr,
 var actions =
 "clone { ct_clear; "
 "inport = outport; outport = \"\"; "
+"eth.dst <-> eth.src; "
 "flags = 0; flags.loopback = 1; " ++
 regs.join("") ++
 "${rEGBIT_EGRESS_LOOPBACK()} = 1; "
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index b6c679907..a0d90e574 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -5890,3 +5890,114 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d
 /.*terminating with signal 15.*/d"])
 AT_CLEANUP
 ])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP(ovn -- DNAT LR hairpin IPv4)
+AT_KEYWORDS(hairpin)
+
+ovn_start
+
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_BR([br-int])
+
+# Set external-ids in br-int needed for ovn-controller
+ovs-vsctl \
+-- set Open_vSwitch . external-ids:system-id=hv1 \
+-- set Open_vSwitch . 
external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
+-- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
+-- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
+-- set bridge br-int fail-mode=secure other-config:disable-in-band=true
+
+start_daemon ovn-controller
+
+# Logical network:
+# Two VMs
+#   * VM1 with IP address 192.168.100.5
+#   * VM2 with IP address 192.168.100.6
+# The VMs connect to logical switch ls1.
+#
+# An external router with IP address 172.18.1.2. We simulate this with a 
network namespace.
+# There will be no traffic going here in this test.
+# The external router connects to logical switch ls-pub
+#
+# One logical router (lr1) connects to ls1 and ls-pub. The router port 
connected to ls-pub is
+# a gateway port.
+#   * The subnet connected to ls1 is 192.168.100.0/24. The Router IP address 
is 192.168.100.1
+#   * The subnet connected to ls-pub is 172.18.1.0/24. The Router IP address 
is 172.168.1.1
+# lr1 has the following attributes:
+#   * It has a "default" static route that sends traffic out the gateway 
router port.
+#   * It has a DNAT rule that translates 172.18.2.10 to 192.168.100.6 (VM2)
+#
+# In this test, we want to ensure that a ping from VM1 to IP address 
172.18.2.10 reaches VM2.
+
+ovn-nbctl ls-add ls1
+ovn-nbctl lsp-add ls1 vm1 -- lsp-set-addresses vm1 "00:00:00:00:00:05 
192.168.100.5"
+ovn-nbctl lsp-add ls1 vm2 -- lsp-set-addresses vm2 "00:00:00:00:00:06 
192.168.100.6"
+
+ovn-nbctl ls-add ls-pub
+ovn-nbctl lsp-add ls-pub ext-router -- lsp-set-addresses ext-router 
"00:00:00:00:01:02 172.18.1.2"
+
+ovn-nbctl lr-add lr1
+ovn-nbctl lrp-add lr1 lr1-ls1 00:00:00:00:00:01 192.168.100.1/24
+ovn-nbctl lsp-add ls1 ls1-lr1  \
+-- lsp-set-type ls1-lr1 router \
+-- lsp-set-addresses ls1-lr1 00:00:00:00:00:01 \
+-- lsp-set-options ls1-lr1 router-port=lr1-ls1
+
+ovn-nbctl lrp-add lr1 lr1-ls-pub 00:00:00:00:01:01 172.18.1.1/24
+ovn-nbctl lrp-set-gateway-chassis lr1-ls-pub hv1
+ovn-nbctl lsp-add ls-pub ls-pub-lr1  \
+-- lsp-set-type ls-pub-lr1 router\
+-- lsp-set-addresses ls-pub-lr1 00:00:00:00:01:01\
+-- lsp-set-options ls-pub-lr1 router-port=lr1-ls-pub
+
+ovn-nbctl lr-nat-add lr1 snat 172.18.1.1 192.168.100.0/24
+ovn-nbctl lr-nat-add lr1 dnat_and_snat 172.18.2.10 192.168.100.6
+ovn-nbctl lr-route-add lr1 0.0.0.0/0 172.18.1.2
+

[ovs-dev] [PATCH ovn v7 2/5] ovn-sb: Remove redundant "nat-addresses" information from Port_Binding.

[Mark Michelson]

Signed-off-by: Mark Michelson 
---
 ovn-sb.xml | 10 --
 1 file changed, 10 deletions(-)

diff --git a/ovn-sb.xml b/ovn-sb.xml
index 258a12b4e..b853a5031 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -3015,16 +3015,6 @@ tcp.flags = RST;
 The chassis in which the port resides.
   
 
-  
-MAC address of the l3gateway port followed by a list of
-SNAT and DNAT external IP addresses.  This is used to send gratuitous
-ARPs for SNAT and DNAT external IP addresses via localnet.
-Example: 80:fa:5b:06:72:b7 158.36.44.22 158.36.44.24.
-This would result in generation of gratuitous ARPs for IP addresses
-158.36.44.22 and 158.36.44.24 with a MAC address of 80:fa:5b:06:72:b7.
-This is used in OVS versions prior to 2.8.
-  
-
   
 MAC address of the l3gateway port followed by a list of
 SNAT and DNAT external IP addresses.  This is used to send gratuitous
-- 
2.29.2

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

[ovs-dev] [PATCH ovn v7 0/5] ARP and Floating IP Fixes

[Mark Michelson]

This patch series aims to fix issues seen in OpenStack deployments when
floating IPs were assigned to routers, and those floating IPs were not
part of any subnet configured on that router.

Originally, this was a two patch series but it has bloomed into a 5
patch series.

Patch 1 fixes the scenario where a VM attempts to reach a floating IP on
the directly connected router. This has been part of this patch series
since v1.

Patch 2 is an incidental fix that removes a redundant paragraph from
documentation.

Patches 3 and 4 work towards pre-allocating MAC_Bindings for known
router addresses. Patch 3 is the northd side, placing all
router_addresses in the connected logical switch port's Port_Binding
record. Patch 4 is the ovn-controller side, adding the MAC_Bindings
based on the Port_Binding's router_addresses.

And Patch 5 addresses the situation for when the pre-allocated
MAC_Bindings cannot be used. For this situation, we will flood the ARP
request if the TPA is for a configured IP address that is outside the
connected routers' subnets.
---
v6 -> v7:
* Patch 3 has been further refined to ensure that router addresses are
  only saved to a switch that is connected to a gateway router port. In
  v6, we ensured the switch was connected to a router that had a gateway
  port. But in v7, we now ensure that the switch is directly connected to
  the gateway port.
* Patch 4 has added a new whitelisted warning message for system tests.
  This is because we can potentially insert the same MAC_Binding record
  twice before we have been notified by the server that the first was
  added.

v5 -> v6:
* Patch 3 now only saves gateway router addresses to the connected
  switch's router_addresses column. Previous versions saved all router
  addresses to all connected switches' columns.
* Patch 5 has two new tests added. One ensures that the priority 90
  flows that flood ARP for unreachable addresses are present. The other
  is a restored system test that ensures that a ping to a floating IP
  outside of the router's subnet succeeds.
* Patch 4 has a small change of types from int to size_t for a loop
  index.

v4 -> v5:
Fixed memory leaks in patch 3 and patch 4. Patches 1, 2,  and 5 are the
same as in v4.
---

Mark Michelson (5):
  northd: Swap src and dst eth addresses in router egress loop.
  ovn-sb: Remove redundant "nat-addresses" information from
Port_Binding.
  northd: Save all router addresses in Port_Bindings
  pinctrl: Add Chassis MAC_Bindings for all router addresses.
  northd: Flood ARPs to routers for "unreachable" addresses.

 controller/ovn-controller.c |   4 +
 controller/pinctrl.c| 300 +---
 controller/pinctrl.h|   1 +
 northd/ovn-northd.8.xml |   8 +
 northd/ovn-northd.c | 378 
 northd/ovn_northd.dl| 153 +++
 ovn-sb.ovsschema|   8 +-
 ovn-sb.xml  |  37 +++-
 tests/ofproto-macros.at |   5 +
 tests/ovn-controller.at | 179 +
 tests/ovn-northd.at | 352 +
 tests/system-ovn.at | 218 +
 12 files changed, 1408 insertions(+), 235 deletions(-)

-- 
2.29.2

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

Re: [ovs-dev] [PATCH v3 14/28] seq-pool: Module for faster ID generation

[Gaëtan Rivet]

On Mon, May 3, 2021, at 14:48, Ilya Maximets wrote:
> On 5/1/21 4:49 PM, Gaëtan Rivet wrote:
> > On Fri, Apr 30, 2021, at 19:31, Ilya Maximets wrote:
> >> On 4/25/21 1:55 PM, Gaetan Rivet wrote:
> >>> The current id-pool module is slow to allocate the
> >>> next valid ID, and can be optimized when restricting
> >>> some properties of the pool.
> >>>
> >>> Those restrictions are:
> >>>
> >>>   * No ability to add a random ID to the pool.
> >>>
> >>>   * A new ID is no more the smallest possible ID.  It is
> >>> however guaranteed to be in the range of
> >>> [base, next_id].  Multiple users of the pool are
> >>> registered, each with a thread-local cache for
> >>> better scalability and the next_id is one after the
> >>> latest ID added to any user cache.
> >>> The allocation range can be written as:
> >>>
> >>>[base, last_alloc + nb-user * cache-size + 1].
> >>>
> >>>   * A user should never free an ID that is not allocated.
> >>> No checks are done and doing so will duplicate the spurious
> >>> ID.  Refcounting or other memory management scheme should
> >>> be used to ensure an object and its ID are only freed once.
> >>>
> >>> This pool is designed to scale reasonably well in multi-thread
> >>> setup.  As it is aimed at being a faster replacement to the
> >>> current id-pool, a benchmark has been implemented alongside
> >>> unit tests.
> >>>
> >>> The benchmark is composed of 4 rounds: 'new', 'del', 'mix', and 'rnd'.
> >>> Respectively
> >>>
> >>>   + 'new': only allocate IDs
> >>>   + 'del': only free IDs
> >>>   + 'mix': allocate, sequential free, then allocate ID.
> >>>   + 'rnd': allocate, random free, allocate ID.
> >>>
> >>> Randomized freeing is done by swapping the latest allocated ID with any
> >>> from the range of currently allocated ID, which is reminiscent of the
> >>> Fisher-Yates shuffle.  This evaluates freeing non-sequential IDs,
> >>> which is the more natural use-case.
> >>>
> >>> For this specific round, the id-pool performance is such that a timeout
> >>> of 10 seconds is added to the benchmark:
> >>>
> >>>$ ./tests/ovstest test-seq-pool benchmark 1 1
> >>>Benchmarking n=1 on 1 thread.
> >>> type\thread:   1Avg
> >>>seq-pool new:   1  1 ms
> >>>seq-pool del:   0  0 ms
> >>>seq-pool mix:   1  1 ms
> >>>seq-pool rnd:   1  1 ms
> >>> id-pool new:   0  0 ms
> >>> id-pool del:   1  1 ms
> >>> id-pool mix:   1  1 ms
> >>> id-pool rnd:1201   1201 ms
> >>>
> >>>$ ./tests/ovstest test-seq-pool benchmark 10 1
> >>>Benchmarking n=10 on 1 thread.
> >>> type\thread:   1Avg
> >>>seq-pool new:   2  2 ms
> >>>seq-pool del:   5  5 ms
> >>>seq-pool mix:   5  5 ms
> >>>seq-pool rnd:   5  5 ms
> >>> id-pool new:   8  8 ms
> >>> id-pool del:   5  5 ms
> >>> id-pool mix:  11 11 ms
> >>> id-pool rnd:  1+ ** ms
> >>>
> >>>$ ./tests/ovstest test-seq-pool benchmark 100 1
> >>>Benchmarking n=100 on 1 thread.
> >>> type\thread:   1Avg
> >>>seq-pool new:  23 23 ms
> >>>seq-pool del:  49 49 ms
> >>>seq-pool mix:  53 53 ms
> >>>seq-pool rnd:  53 53 ms
> >>> id-pool new: 190190 ms
> >>> id-pool del: 173173 ms
> >>> id-pool mix: 273273 ms
> >>> id-pool rnd:  10042+ ** ms
> >>>
> >>>$ ./tests/ovstest test-seq-pool benchmark 100 2
> >>>Benchmarking n=100 on 2 threads.
> >>> type\thread:   1  2Avg
> >>>seq-pool new:  40 39 39 ms
> >>>seq-pool del:  33 33 33 ms
> >>>seq-pool mix:  89 91 90 ms
> >>>seq-pool rnd: 146151148 ms
> >>> id-pool new: 485485485 ms
> >>> id-pool del: 541542541 ms
> >>> id-pool mix: 550600575 ms
> >>> id-pool rnd:  10048+ 10003+ ** ms
> >>>
> >>>$ ./tests/ovstest test-seq-pool benchmark 100 4
> >>>Benchmarking n=100 on 4 threads.
> >>> type\thread:   1  2  3  4Avg
> >>>seq-pool new:  40 39 40 40 39 ms
> >>>seq-pool del:  24 28 28 30 27 ms
> >>>seq-pool mix:  60 63 69 69 65 ms
> >>>seq-pool rnd: 195197202202199 ms
> >>> id-pool new: 478471482485479 ms
> >>> id-pool del: 474469467474471 ms
> >>> id-pool mix: 558558611545568 ms
> >>> id-pool rnd:  10121+ 10076+ 10030+ 10167+ ** ms
> >>
> >> Exactly same question here.  The new data structure doesn't provide
> >> any guarantees and checks.  It doesn't allocate smallest id.
> >> When why not just an array + spinlock?   We're already using this
> >> schema in lib/netdev-afxdp-pool.c and it 

Re: [ovs-dev] [PATCH v2] tests: Add PMD auto load balance unit tests.

[Kevin Traynor]

Ping. Any objection to adding these unit tests?

On 16/03/2021 15:45, Kevin Traynor wrote:
> These tests focus on enabling/disabling and user parameters.
> 
> Co-Authored-by: David Marchand 
> Signed-off-by: David Marchand 
> Signed-off-by: Kevin Traynor 
> 
> ---
> v2:
> - Remove above max documented interval test
> - Add David's code to combine param checks and add as co-author
> ---
>  tests/alb.at   | 218 +
>  tests/automake.mk  |   1 +
>  tests/testsuite.at |   1 +
>  3 files changed, 220 insertions(+)
>  create mode 100644 tests/alb.at
> 
> diff --git a/tests/alb.at b/tests/alb.at
> new file mode 100644
> index 0..0ea1bbdd1
> --- /dev/null
> +++ b/tests/alb.at
> @@ -0,0 +1,218 @@
> +AT_BANNER([PMD Auto Load Balance])
> +
> +m4_divert_push([PREPARE_TESTS])
> +
> +get_log_line_num () {
> +LINENUM=$(cat ovs-vswitchd.log | wc -l | tr -d [[:blank:]])
> +}
> +
> +m4_divert_pop([PREPARE_TESTS])
> +
> +m4_define([DUMMY_NUMA], [--dummy-numa="0,0"])
> +
> +dnl CHECK_ALB_PARAM([param], [value], [+line])
> +dnl
> +dnl Waits for ALB logs for 'param' in logs and checks if value matches
> +dnl 'value'. Checking starts from line number 'line' in ovs-vswithd.log.
> +m4_define([CHECK_ALB_PARAM], [
> +line_st=$3
> +if [[ -z "$line_st" ]]
> +then
> +line_st="+0"
> +fi
> +OVS_WAIT_UNTIL([tail -n $line_st ovs-vswitchd.log | grep "PMD auto load 
> balance $1 set to"])
> +AT_CHECK([tail -n $line_st ovs-vswitchd.log | sed -n "s#.*\(PMD auto 
> load balance $1 set to.*\)#\1#p" | tail -1], [0], [dnl
> +PMD auto load balance $1 set to $2
> +])
> +])
> +
> +AT_SETUP([ALB - default state])
> +OVS_VSWITCHD_START
> +OVS_WAIT_UNTIL([grep "PMD auto load balance is disabled" ovs-vswitchd.log])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +AT_SETUP([ALB - enable/disable])
> +OVS_VSWITCHD_START([add-port br0 p0 \
> +-- set Interface p0 type=dummy-pmd options:n_rxq=3 \
> +-- set Open_vSwitch . other_config:pmd-cpu-mask=3 \
> +-- set open_vswitch . other_config:pmd-auto-lb="true"],
> +   [], [], [DUMMY_NUMA])
> +OVS_WAIT_UNTIL([grep "PMD auto load balance is enabled" ovs-vswitchd.log])
> +
> +get_log_line_num
> +AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb="false"])
> +OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load 
> balance is disabled"])
> +
> +get_log_line_num
> +AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb="true"])
> +OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load 
> balance is enabled"])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +AT_SETUP([ALB - min num PMD/RxQ])
> +OVS_VSWITCHD_START([add-port br0 p0 \
> +-- set Interface p0 type=dummy-pmd options:n_rxq=2 \
> +-- set Open_vSwitch . other_config:pmd-cpu-mask=1 \
> +-- set open_vswitch . other_config:pmd-auto-lb="true"],
> +   [], [], [DUMMY_NUMA])
> +OVS_WAIT_UNTIL([grep "PMD auto load balance is disabled" ovs-vswitchd.log])
> +
> +# Add more PMD
> +AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:pmd-cpu-mask=0x3])
> +OVS_WAIT_UNTIL([grep "There are 2 pmd threads on numa node" 
> ovs-vswitchd.log])
> +
> +# Add one more rxq to have 2 rxq on a PMD
> +get_log_line_num
> +AT_CHECK([ovs-vsctl set interface p0 options:n_rxq=3])
> +OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load 
> balance is enabled"])
> +
> +# Reduce PMD
> +get_log_line_num
> +AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:pmd-cpu-mask=0x1])
> +OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load 
> balance is disabled"])
> +
> +# Check logs when try to enable but min PMD/RxQ prevents
> +get_log_line_num
> +AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb="false"])
> +OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load 
> balance is disabled"])
> +get_log_line_num
> +AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb="true"])
> +OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load 
> balance is disabled"])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +AT_SETUP([ALB - PMD/RxQ assignment type])
> +OVS_VSWITCHD_START([add-port br0 p0 \
> +-- set Interface p0 type=dummy-pmd options:n_rxq=3 \
> +-- set Open_vSwitch . other_config:pmd-cpu-mask=3 \
> +-- set open_vswitch . other_config:pmd-auto-lb="true"],
> +   [], [], [DUMMY_NUMA])
> +OVS_WAIT_UNTIL([grep "PMD auto load balance is enabled" ovs-vswitchd.log])
> +
> +# Change assignment type
> +get_log_line_num
> +AT_CHECK([ovs-vsctl set Open_vSwitch . 
> other_config:pmd-rxq-assign=roundrobin])
> +OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load 
> balance is disabled"])
> +
> +# Change back assignment type
> 

Re: [ovs-dev] [PATCH v3] dpif-netdev: Allow PMD auto load balance with cross-numa.

[Kevin Traynor]

On 04/05/2021 17:12, Yogananth Subramanian wrote:
> Hello Everyone

Hi Yogananth,

> I was able to verify the patch, below are the  observed behavior of  OVS
> 2.15 release and OVS from git with the patch.
> OVS 2.15 release :
> 
> PMD auto balance aborted:
> 
> 2021-04-27T08:54:54.770Z|00406|dpif_netdev|DBG|PMD auto lb dry run.
> Current: Core 3, usage 51
> 
> 2021-04-27T08:54:54.770Z|00407|dpif_netdev|DBG|PMD auto lb dry run.
> Current: Core 5, usage 51
> 
> 2021-04-27T08:54:54.770Z|00408|dpif_netdev|DBG|PMD auto lb dry run.
> Current: Core 27, usage 96
> 
> 2021-04-27T08:54:54.770Z|00409|dpif_netdev|DBG|PMD auto lb dry run.
> Current: Core 29, usage 93
> 
> 2021-04-27T08:54:54.770Z|00410|dpif_netdev|DBG|PMD auto lb dry run.
> Aborting due to cross-numa polling.

^ right, this is where without the patch any cross-numa polling prevents
the alb feature from working.

> OVS from git with PDM cross numa rebalance commit:
> 
> PMD auto balance triggered:
> 
> 2021-04-27T11:32:17.985Z|00805|dpif_netdev|INFO|PMD auto lb dry run.
> requesting datapath reconfigure.
> 
> 2021-04-27T11:32:17.985Z|00806|dpif_netdev|WARN|There's no available
> (non-isolated) pmd thread on numa node 0. Queue 0 on port 'dpdk-p1' will be
> assigned to the pmd on core 3 (numa node 1). Expect reduced performance.
> 
> 2021-04-27T11:32:17.985Z|00807|dpif_netdev|WARN|There's no available
> (non-isolated) pmd thread on numa node 0. Queue 0 on port 'dpdk-p0' will be
> assigned to the pmd on core 29 (numa node 1). Expect reduced performance.
> 
> 2021-04-27T11:32:17.985Z|00808|dpif_netdev|WARN|There's no available
> (non-isolated) pmd thread on numa node 0. Queue 2 on port 'dpdk-p0' will be
> assigned to the pmd on core 27 (numa node 1). Expect reduced performance.
> 
> 2021-04-27T11:32:17.985Z|00809|dpif_netdev|WARN|There's no available
> (non-isolated) pmd thread on numa node 0. Queue 1 on port 'dpdk-p0' will be
> assigned to the pmd on core 5 (numa node 1). Expect reduced performance.
> 
> 2021-04-27T11:32:17.985Z|00810|dpif_netdev|WARN|There's no available
> (non-isolated) pmd thread on numa node 0. Queue 2 on port 'dpdk-p1' will be
> assigned to the pmd on core 5 (numa node 1). Expect reduced performance.
> 
> 2021-04-27T11:32:17.985Z|00811|dpif_netdev|WARN|There's no available
> (non-isolated) pmd thread on numa node 0. Queue 1 on port 'dpdk-p1' will be
> assigned to the pmd on core 27 (numa node 1). Expect reduced performance.
> 

^ yes, this indicates that the alb has triggered a rebalance while there
is cross-numa polling limited to pmds on one numa node, which is the
intended behaviour.

btw, just to note for future mails that plaintext is preferred on the
mailing list.

Thanks for testing,
Kevin.

> 
> Before rebalance
> 
> After rebalance
> 
> pmd thread numa_id 1 core_id 3:
> 
>   isolated : false
> 
>   port: dpdk-p1   queue-id:  0 (enabled)   pmd usage: 46 %
> 
> pmd thread numa_id 1 core_id 5:
> 
>   isolated : false
> 
>   port: dpdk-p0   queue-id:  0 (enabled)   pmd usage: 32 %
> 
>   port: dpdk-p0   queue-id:  1 (enabled)   pmd usage: 26 %
> 
> pmd thread numa_id 1 core_id 27:
> 
>   isolated : false
> 
>   port: dpdk-p0   queue-id:  2 (enabled)   pmd usage: 35 %
> 
>   port: dpdk-p1   queue-id:  2 (enabled)   pmd usage: 18 %
> 
> pmd thread numa_id 1 core_id 29:
> 
>   isolated : false
> 
>   port: dpdk-p1   queue-id:  1 (enabled)   pmd usage: 13 %
> 
> pmd thread numa_id 1 core_id 3:
> 
>   isolated : false
> 
>   port: dpdk-p1   queue-id:  0 (enabled)   pmd usage: 76 %
> 
> pmd thread numa_id 1 core_id 5:
> 
>   isolated : false
> 
>   port: dpdk-p0   queue-id:  1 (enabled)   pmd usage: 47 %
> 
>   port: dpdk-p1   queue-id:  2 (enabled)   pmd usage: 23 %
> 
> pmd thread numa_id 1 core_id 27:
> 
>   isolated : false
> 
>   port: dpdk-p0   queue-id:  0 (enabled)   pmd usage: 67 %
> 
>   port: dpdk-p1   queue-id:  1 (enabled)   pmd usage: 19 %
> 
> pmd thread numa_id 1 core_id 29:
> 
>   isolated : false
> 
>   port: dpdk-p0   queue-id:  2 (enabled)   pmd usage: 54 %
> 
> 
> 
> On Mon, Mar 22, 2021 at 5:31 PM Ilya Maximets  wrote:
> 
>> On 3/18/21 12:34 PM, Kevin Traynor wrote:
>>> Previously auto load balance did not trigger a reassignment when
>>> there was any cross-numa polling as an rxq could be polled from a
>>> different numa after reassign and it could impact estimates.
>>>
>>> In the case where there is only one numa with pmds available, the
>>> same numa will always poll before and after reassignment, so estimates
>>> are valid. Allow PMD auto load balance to trigger a reassignment in
>>> this case.
>>>
>>> Acked-by: Eelco Chaudron 
>>> Acked-by: David Marchand 
>>> Tested-by: Sunil Pai G 
>>> Acked-by: Flavio Leitner 
>>> Signed-off-by: Kevin Traynor 
>>> ---
>>> https://github.com/kevintraynor/ovs/actions/runs/664214192
>>> v3:
>>> - Updated docs and logs as per Ilya suggestion
>>> v2:
>>> - Same logic as v1, combined 

Re: [ovs-dev] [PATCH v3] Don't mangle shebangs when building DKMS RPM package

[Guzowski Adrian via dev]

W dniu śro, 05.05.2021 o godzinie 13∶16 +0200, użytkownik Ilya Maximets napisał:
>
Spółka wpisana do rejestru przedsiębiorców w Sądzie Rejonowym dla m. st. 
Warszawy, XIII Wydział Gospodarczy, pod numerem KRS: 044577, kapitał 
zakładowy: 576 854 559 PLN, kapitał opłacony w całości, NIP: 527-010-45-68, 
BDO: 000250055, EXATEL SA, ul. Perkuna 47, 04-164 Warszawa.

Niniejsza wiadomość jest własnością EXATEL SA i może zawierać informacje poufne 
i/lub prawnie chronione. Jeśli nie są Państwo właściwym adresatem (lub 
otrzymali Państwo tę wiadomość na skutek pomyłki), prosimy o tym fakcie 
niezwłocznie poinformować nadawcę i usunąć otrzymaną wiadomość. Każde 
nieautoryzowane kopiowanie, ujawnianie lub rozpowszechnianie załączonej 
informacji jest zabronione.

This e-mail is a property of EXATEL SA and may contain confidential and/or 
privileged information. If you are not the intended recipient (or have received 
this e-mail by mistake) please notify the sender immediately and destroy this 
e-mail. Any unauthorized copying, disclosure or distribution of the material in 
this e-mail is strictly forbidden.
On 5/5/21 12:53 PM, Guzowski Adrian via dev wrote:
> > While building the package, some .in files are being subject to shebang
> > substitution, but the process fails, because given scripts have
> > placeholders in place of shebangs. In order to fix the issue, don't mangle
> > shebangs in this specific package.
> >
> > Signed-off-by: Guzowski Adrian 
> > ---
> >  rhel/openvswitch-dkms.spec.in | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/rhel/openvswitch-dkms.spec.in b/rhel/openvswitch-dkms.spec.in
> > index a47c038fd..5a57e4d48 100644
> > --- a/rhel/openvswitch-dkms.spec.in
> > +++ b/rhel/openvswitch-dkms.spec.in
> > @@ -27,6 +27,8 @@ BuildRoot:%(mktemp -ud 
> > %{_tmppath}/%{name}-%{version}-%{release}-
> > XX)
> >  # conflicts with the openvswitch-debuginfo package for OVS userspace).
> >  %undefine _enable_debug_packages
> >
> > +# Disable shebangs mangling
> > +%undefine __brp_mangle_shebangs
> >
> >  %description
> >  Open vSwitch Linux kernel module.
> > --
> > 2.30.2
> >
> >
> >
> > Spółka wpisana do rejestru przedsiębiorców w Sądzie Rejonowym dla m. st. 
> > Warszawy, XIII Wydział
> > Gospodarczy, pod numerem KRS: 044577, kapitał zakładowy: 576 854 559 
> > PLN, kapitał opłacony w
> > całości, NIP: 527-010-45-68, BDO: 000250055, EXATEL SA, ul. Perkuna 47, 
> > 04-164 Warszawa.
> >
> > Niniejsza wiadomość jest własnością EXATEL SA i może zawierać informacje 
> > poufne i/lub prawnie
> > chronione. Jeśli nie są Państwo właściwym adresatem (lub otrzymali Państwo 
> > tę wiadomość na
> > skutek pomyłki), prosimy o tym fakcie niezwłocznie poinformować nadawcę i 
> > usunąć otrzymaną
> > wiadomość. Każde nieautoryzowane kopiowanie, ujawnianie lub 
> > rozpowszechnianie załączonej
> > informacji jest zabronione.
> >
> > This e-mail is a property of EXATEL SA and may contain confidential and/or 
> > privileged
> > information. If you are not the intended recipient (or have received this 
> > e-mail by mistake)
> > please notify the sender immediately and destroy this e-mail. Any 
> > unauthorized copying,
> > disclosure or distribution of the material in this e-mail is strictly 
> > forbidden.
>
> Hi.  Could you, please, re-send your patches without the above
> confidentiality notice?  It makes no sense while sending emails to
> the public mail-list and it kind of conflicts with the opensource
> patch submission process.  I'm not an expert in legal aspects, but
> I think we cannot accept a patch that explicitly prohibits distribution.
>
> Best regards, Ilya Maximets.
Hello. I've requested the removal from my IT department and waiting for a 
response - this is a
preconfigured notice that every employee at our organization has.

As for the legal part, the dev mailing list is the intended recipient so there 
should not be any
issue with it, but I am too, not an expert in this matter, so if that's in fact 
a problem, I can
submit it via GitHub PR.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] Don't mangle shebangs when building DKMS RPM package

[Ilya Maximets]

On 5/5/21 12:53 PM, Guzowski Adrian via dev wrote:
> While building the package, some .in files are being subject to shebang
> substitution, but the process fails, because given scripts have
> placeholders in place of shebangs. In order to fix the issue, don't mangle
> shebangs in this specific package.
> 
> Signed-off-by: Guzowski Adrian 
> ---
>  rhel/openvswitch-dkms.spec.in | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/rhel/openvswitch-dkms.spec.in b/rhel/openvswitch-dkms.spec.in
> index a47c038fd..5a57e4d48 100644
> --- a/rhel/openvswitch-dkms.spec.in
> +++ b/rhel/openvswitch-dkms.spec.in
> @@ -27,6 +27,8 @@ BuildRoot:%(mktemp -ud 
> %{_tmppath}/%{name}-%{version}-%{release}-XX)
>  # conflicts with the openvswitch-debuginfo package for OVS userspace).
>  %undefine _enable_debug_packages
> 
> +# Disable shebangs mangling
> +%undefine __brp_mangle_shebangs
> 
>  %description
>  Open vSwitch Linux kernel module.
> --
> 2.30.2
> 
> 
> 
> Spółka wpisana do rejestru przedsiębiorców w Sądzie Rejonowym dla m. st. 
> Warszawy, XIII Wydział Gospodarczy, pod numerem KRS: 044577, kapitał 
> zakładowy: 576 854 559 PLN, kapitał opłacony w całości, NIP: 527-010-45-68, 
> BDO: 000250055, EXATEL SA, ul. Perkuna 47, 04-164 Warszawa.
> 
> Niniejsza wiadomość jest własnością EXATEL SA i może zawierać informacje 
> poufne i/lub prawnie chronione. Jeśli nie są Państwo właściwym adresatem (lub 
> otrzymali Państwo tę wiadomość na skutek pomyłki), prosimy o tym fakcie 
> niezwłocznie poinformować nadawcę i usunąć otrzymaną wiadomość. Każde 
> nieautoryzowane kopiowanie, ujawnianie lub rozpowszechnianie załączonej 
> informacji jest zabronione.
> 
> This e-mail is a property of EXATEL SA and may contain confidential and/or 
> privileged information. If you are not the intended recipient (or have 
> received this e-mail by mistake) please notify the sender immediately and 
> destroy this e-mail. Any unauthorized copying, disclosure or distribution of 
> the material in this e-mail is strictly forbidden.

Hi.  Could you, please, re-send your patches without the above
confidentiality notice?  It makes no sense while sending emails to
the public mail-list and it kind of conflicts with the opensource
patch submission process.  I'm not an expert in legal aspects, but
I think we cannot accept a patch that explicitly prohibits distribution.

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

[ovs-dev] [PATCH v3] Don't mangle shebangs when building DKMS RPM package

[Guzowski Adrian via dev]

While building the package, some .in files are being subject to shebang
substitution, but the process fails, because given scripts have
placeholders in place of shebangs. In order to fix the issue, don't mangle
shebangs in this specific package.

Signed-off-by: Guzowski Adrian 
---
 rhel/openvswitch-dkms.spec.in | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/rhel/openvswitch-dkms.spec.in b/rhel/openvswitch-dkms.spec.in
index a47c038fd..5a57e4d48 100644
--- a/rhel/openvswitch-dkms.spec.in
+++ b/rhel/openvswitch-dkms.spec.in
@@ -27,6 +27,8 @@ BuildRoot:%(mktemp -ud 
%{_tmppath}/%{name}-%{version}-%{release}-XX)
 # conflicts with the openvswitch-debuginfo package for OVS userspace).
 %undefine _enable_debug_packages

+# Disable shebangs mangling
+%undefine __brp_mangle_shebangs

 %description
 Open vSwitch Linux kernel module.
--
2.30.2



Spółka wpisana do rejestru przedsiębiorców w Sądzie Rejonowym dla m. st. 
Warszawy, XIII Wydział Gospodarczy, pod numerem KRS: 044577, kapitał 
zakładowy: 576 854 559 PLN, kapitał opłacony w całości, NIP: 527-010-45-68, 
BDO: 000250055, EXATEL SA, ul. Perkuna 47, 04-164 Warszawa.

Niniejsza wiadomość jest własnością EXATEL SA i może zawierać informacje poufne 
i/lub prawnie chronione. Jeśli nie są Państwo właściwym adresatem (lub 
otrzymali Państwo tę wiadomość na skutek pomyłki), prosimy o tym fakcie 
niezwłocznie poinformować nadawcę i usunąć otrzymaną wiadomość. Każde 
nieautoryzowane kopiowanie, ujawnianie lub rozpowszechnianie załączonej 
informacji jest zabronione.

This e-mail is a property of EXATEL SA and may contain confidential and/or 
privileged information. If you are not the intended recipient (or have received 
this e-mail by mistake) please notify the sender immediately and destroy this 
e-mail. Any unauthorized copying, disclosure or distribution of the material in 
this e-mail is strictly forbidden.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] Add ability to override default Release suffix in RPM packages

[Guzowski Adrian via dev]

W dniu śro, 05.05.2021 o godzinie 12∶33 +0200, użytkownik Timothy Redaelli 
napisał:
> On Tue, 4 May 2021 06:24:33 +

Spółka wpisana do rejestru przedsiębiorców w Sądzie Rejonowym dla m. st. 
Warszawy, XIII Wydział Gospodarczy, pod numerem KRS: 044577, kapitał 
zakładowy: 576 854 559 PLN, kapitał opłacony w całości, NIP: 527-010-45-68, 
BDO: 000250055, EXATEL SA, ul. Perkuna 47, 04-164 Warszawa.

Niniejsza wiadomość jest własnością EXATEL SA i może zawierać informacje poufne 
i/lub prawnie chronione. Jeśli nie są Państwo właściwym adresatem (lub 
otrzymali Państwo tę wiadomość na skutek pomyłki), prosimy o tym fakcie 
niezwłocznie poinformować nadawcę i usunąć otrzymaną wiadomość. Każde 
nieautoryzowane kopiowanie, ujawnianie lub rozpowszechnianie załączonej 
informacji jest zabronione.

This e-mail is a property of EXATEL SA and may contain confidential and/or 
privileged information. If you are not the intended recipient (or have received 
this e-mail by mistake) please notify the sender immediately and destroy this 
e-mail. Any unauthorized copying, disclosure or distribution of the material in 
this e-mail is strictly forbidden.
> Guzowski Adrian via dev  wrote:
>
> > In some cases, like building OvS packages in Jenkins, it may be
> > useful to set a custom version suffix that will correspond with
> > job's build number, etc. Currently, version number is explicitly
> > set to 1. This change adds a define "release_number" that may be
> > overridden during package bulding process:
> >
> > rpmbuild -ba --define="release_number X" ...
> >
> > Signed-off-by: Adrian Guzowski 
> > ---
> >  rhel/kmod-openvswitch-rhel6.spec.in  | 3 ++-
> >  rhel/openvswitch-dkms.spec.in| 4 ++--
> >  rhel/openvswitch-fedora.spec.in  | 4 +++-
> >  rhel/openvswitch-kmod-fedora.spec.in | 4 +++-
> >  rhel/openvswitch.spec.in | 4 +++-
> >  5 files changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/rhel/kmod-openvswitch-rhel6.spec.in 
> > b/rhel/kmod-openvswitch-rhel6.spec.in
> > index 7d3d9b498..de69863d7 100644
> > --- a/rhel/kmod-openvswitch-rhel6.spec.in
> > +++ b/rhel/kmod-openvswitch-rhel6.spec.in
> > @@ -9,10 +9,11 @@
> >  # without warranty of any kind.
> >
> >  %define oname openvswitch
> > +%{!?release_number:%define release_number 1}
> >
> >  Name:   kmod-%{oname}
> >  Version:@VERSION@
> > -Release:1%{?dist}
> > +Release:%{release_number}%{?dist}
>
> Hi,
> %{?release_number}%{?!release_number:1}
> ^ this pattern is more common, instead of defining if not defined, but I
> don't have a strong preference about it
>
I was wondering which one to use and decidec to follow the style of defining 
package name (one
before the added line) - just in case the value would be reused. I don't have a 
strong preference
for it either, can be changed accordingly if anyone objects.

> I also see that "0-day Robot" reported an error since the From: field
> of you email is ovs-dev@openvswitch.org instead of your email address
>
Actually, that's the error from the first attempt of sending a patch to OVS - 
turned out that our
company mail server puts From: in the form of {last-name} {first-name}, where 
my local git settings
were {first-name} {last-name}, so my patch came from "Guzowski Adrian", but was 
signed off by
"Adrian Guzowski". Once I'll get through the process of submitting the first 
patch (fixing shebangs
in DKMS RPM package), I'll send out remaining fixed patches.

> >  Summary:Open vSwitch kernel module
> >
> >  Group:  System/Kernel
> > diff --git a/rhel/openvswitch-dkms.spec.in b/rhel/openvswitch-dkms.spec.in
> > index a47c038fd..c8a978a17 100644
> > --- a/rhel/openvswitch-dkms.spec.in
> > +++ b/rhel/openvswitch-dkms.spec.in
> > @@ -8,10 +8,11 @@
> >  # without warranty of any kind.
> >
> >  %define oname openvswitch
> > +%{!?release_number:%define release_number 1}
> >
> >  Name: %{oname}-dkms
> >  Version:  @VERSION@
> > -Release:  1%{?dist}
> > +Release:  %{release_number}%{?dist}
> >  Summary:  Open vSwitch kernel module
> >
> >  Group:System/Kernel
> > @@ -27,7 +28,6 @@ BuildRoot:%(mktemp -ud 
> > %{_tmppath}/%{name}-%{version}-%{release}-
> > XX)
> >  # conflicts with the openvswitch-debuginfo package for OVS userspace).
> >  %undefine _enable_debug_packages
> >
> > -
> >  %description
> >  Open vSwitch Linux kernel module.
> >
> > diff --git a/rhel/openvswitch-fedora.spec.in 
> > b/rhel/openvswitch-fedora.spec.in
> > index e457fa679..90f242b1f 100644
> > --- a/rhel/openvswitch-fedora.spec.in
> > +++ b/rhel/openvswitch-fedora.spec.in
> > @@ -44,6 +44,8 @@
> >  %define _rundir /run
> >  %endif
> >
> > +%{!?release_number:%define release_number 1}
> > +
> >  Name: openvswitch
> >  Summary: Open vSwitch
> >  Group: System Environment/Daemons
> > @@ -54,7 +56,7 @@ Version: @VERSION@
> >  # lib/sflow*.[ch] files are SISSL
> >  # datapath/ is GPLv2 (although not built 

Re: [ovs-dev] [PATCH v2] rpm: Don't mangle shebangs when building DKMS RPM package

[Guzowski Adrian via dev]

W dniu śro, 05.05.2021 o godzinie 12∶20 +0200, użytkownik Timothy Redaelli 
napisał:
> On Wed, 5 May 2021 05:37:41 +

Spółka wpisana do rejestru przedsiębiorców w Sądzie Rejonowym dla m. st. 
Warszawy, XIII Wydział Gospodarczy, pod numerem KRS: 044577, kapitał 
zakładowy: 576 854 559 PLN, kapitał opłacony w całości, NIP: 527-010-45-68, 
BDO: 000250055, EXATEL SA, ul. Perkuna 47, 04-164 Warszawa.

Niniejsza wiadomość jest własnością EXATEL SA i może zawierać informacje poufne 
i/lub prawnie chronione. Jeśli nie są Państwo właściwym adresatem (lub 
otrzymali Państwo tę wiadomość na skutek pomyłki), prosimy o tym fakcie 
niezwłocznie poinformować nadawcę i usunąć otrzymaną wiadomość. Każde 
nieautoryzowane kopiowanie, ujawnianie lub rozpowszechnianie załączonej 
informacji jest zabronione.

This e-mail is a property of EXATEL SA and may contain confidential and/or 
privileged information. If you are not the intended recipient (or have received 
this e-mail by mistake) please notify the sender immediately and destroy this 
e-mail. Any unauthorized copying, disclosure or distribution of the material in 
this e-mail is strictly forbidden.
> Guzowski Adrian via dev  wrote:
>
> > While building the package, some .in files are being subject to shebang
> > substitution, but the process fails, because given scripts have
> > placeholders in place of shebangs. In order to fix the issue, don't mangle
> > shebangs in this specific package.
> >
> > Signed-off-by: Guzowski Adrian 
> > ---
> >  rhel/openvswitch-dkms.spec.in | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/rhel/openvswitch-dkms.spec.in b/rhel/openvswitch-dkms.spec.in
> > index a47c038fd..d9955f361 100644
> > --- a/rhel/openvswitch-dkms.spec.in
> > +++ b/rhel/openvswitch-dkms.spec.in
> > @@ -27,6 +27,8 @@ BuildRoot:%(mktemp -ud 
> > %{_tmppath}/%{name}-%{version}-%{release}-
> > XX)
> >  # conflicts with the openvswitch-debuginfo package for OVS userspace).
> >  %undefine _enable_debug_packages
> >
> > +# Exclude input files from mangling
> > +%global __brp_mangle_shebangs_exclude_from ^/usr/src/.*$
> >
> >  %description
> >  Open vSwitch Linux kernel module.
> > --
> > 2.30.2
>
> Hi,
> since almost any file is under /usr/src, you could just disable the
> mangling feature entirely [1] instead of specify a regexp:
>
> %undefine __brp_mangle_shebangs
>
> [1]
> https://docs.fedoraproject.org/en-US/packaging-guidelines/#_shebang_lines
>
Thanks for the referrence, I'll update the patch and send out v3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] Add ability to override default Release suffix in RPM packages

[Timothy Redaelli]

On Tue, 4 May 2021 06:24:33 +
Guzowski Adrian via dev  wrote:

> In some cases, like building OvS packages in Jenkins, it may be
> useful to set a custom version suffix that will correspond with
> job's build number, etc. Currently, version number is explicitly
> set to 1. This change adds a define "release_number" that may be
> overridden during package bulding process:
> 
> rpmbuild -ba --define="release_number X" ...
> 
> Signed-off-by: Adrian Guzowski 
> ---
>  rhel/kmod-openvswitch-rhel6.spec.in  | 3 ++-
>  rhel/openvswitch-dkms.spec.in| 4 ++--
>  rhel/openvswitch-fedora.spec.in  | 4 +++-
>  rhel/openvswitch-kmod-fedora.spec.in | 4 +++-
>  rhel/openvswitch.spec.in | 4 +++-
>  5 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/rhel/kmod-openvswitch-rhel6.spec.in 
> b/rhel/kmod-openvswitch-rhel6.spec.in
> index 7d3d9b498..de69863d7 100644
> --- a/rhel/kmod-openvswitch-rhel6.spec.in
> +++ b/rhel/kmod-openvswitch-rhel6.spec.in
> @@ -9,10 +9,11 @@
>  # without warranty of any kind.
> 
>  %define oname openvswitch
> +%{!?release_number:%define release_number 1}
> 
>  Name:   kmod-%{oname}
>  Version:@VERSION@
> -Release:1%{?dist}
> +Release:%{release_number}%{?dist}

Hi,
%{?release_number}%{?!release_number:1}
^ this pattern is more common, instead of defining if not defined, but I
don't have a strong preference about it

I also see that "0-day Robot" reported an error since the From: field
of you email is ovs-dev@openvswitch.org instead of your email address

>  Summary:Open vSwitch kernel module
> 
>  Group:  System/Kernel
> diff --git a/rhel/openvswitch-dkms.spec.in b/rhel/openvswitch-dkms.spec.in
> index a47c038fd..c8a978a17 100644
> --- a/rhel/openvswitch-dkms.spec.in
> +++ b/rhel/openvswitch-dkms.spec.in
> @@ -8,10 +8,11 @@
>  # without warranty of any kind.
> 
>  %define oname openvswitch
> +%{!?release_number:%define release_number 1}
> 
>  Name: %{oname}-dkms
>  Version:  @VERSION@
> -Release:  1%{?dist}
> +Release:  %{release_number}%{?dist}
>  Summary:  Open vSwitch kernel module
> 
>  Group:System/Kernel
> @@ -27,7 +28,6 @@ BuildRoot:%(mktemp -ud 
> %{_tmppath}/%{name}-%{version}-%{release}-XX)
>  # conflicts with the openvswitch-debuginfo package for OVS userspace).
>  %undefine _enable_debug_packages
> 
> -
>  %description
>  Open vSwitch Linux kernel module.
> 
> diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
> index e457fa679..90f242b1f 100644
> --- a/rhel/openvswitch-fedora.spec.in
> +++ b/rhel/openvswitch-fedora.spec.in
> @@ -44,6 +44,8 @@
>  %define _rundir /run
>  %endif
> 
> +%{!?release_number:%define release_number 1}
> +
>  Name: openvswitch
>  Summary: Open vSwitch
>  Group: System Environment/Daemons
> @@ -54,7 +56,7 @@ Version: @VERSION@
>  # lib/sflow*.[ch] files are SISSL
>  # datapath/ is GPLv2 (although not built into any of the binary packages)
>  License: ASL 2.0 and LGPLv2+ and SISSL
> -Release: 1%{?dist}
> +Release: %{release_number}%{?dist}
>  Source: http://openvswitch.org/releases/%{name}-%{version}.tar.gz
> 
>  BuildRequires: gcc gcc-c++
> diff --git a/rhel/openvswitch-kmod-fedora.spec.in 
> b/rhel/openvswitch-kmod-fedora.spec.in
> index ff190064f..e5f78701f 100644
> --- a/rhel/openvswitch-kmod-fedora.spec.in
> +++ b/rhel/openvswitch-kmod-fedora.spec.in
> @@ -25,6 +25,8 @@
>  #define kernel %{kernel_source}
>  %{?kversion:%define kernel %kversion}
> 
> +%{!?release_number:%define release_number 1}
> +
>  Name: openvswitch-kmod
>  Summary: Open vSwitch Kernel Modules
>  Group: System Environment/Daemons
> @@ -34,7 +36,7 @@ Version: @VERSION@
> 
>  # The entire source code is ASL 2.0 except datapath/ which is GPLv2
>  License: GPLv2
> -Release: 1%{?dist}
> +Release: %{release_number}%{?dist}
>  Source: openvswitch-%{version}.tar.gz
>  #Source1: openvswitch-init
>  Buildroot: /tmp/openvswitch-xen-rpm
> diff --git a/rhel/openvswitch.spec.in b/rhel/openvswitch.spec.in
> index ee8b3c9ea..220e5c747 100644
> --- a/rhel/openvswitch.spec.in
> +++ b/rhel/openvswitch.spec.in
> @@ -21,6 +21,8 @@
>  # testing out of tree kernel modules the appropriate openvswitch-kmod
>  # package should be installed first.
> 
> +%{!?release_number:%define release_number 1}
> +
>  Name: openvswitch
>  Summary: Open vSwitch daemon/database/utilities
>  Group: System Environment/Daemons
> @@ -29,7 +31,7 @@ Vendor: Nicira, Inc.
>  Version: @VERSION@
> 
>  License: ASL 2.0
> -Release: 1
> +Release: %{release_number}%{?dist}
>  Source: openvswitch-%{version}.tar.gz
>  Buildroot: /tmp/openvswitch-rpm
>  Requires: logrotate, hostname, python >= 2.7, python-six
> --
> 2.30.2

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

Re: [ovs-dev] [PATCH v2] rpm: Don't mangle shebangs when building DKMS RPM package

[Timothy Redaelli]

On Wed, 5 May 2021 05:37:41 +
Guzowski Adrian via dev  wrote:

> While building the package, some .in files are being subject to shebang
> substitution, but the process fails, because given scripts have
> placeholders in place of shebangs. In order to fix the issue, don't mangle
> shebangs in this specific package.
> 
> Signed-off-by: Guzowski Adrian 
> ---
>  rhel/openvswitch-dkms.spec.in | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/rhel/openvswitch-dkms.spec.in b/rhel/openvswitch-dkms.spec.in
> index a47c038fd..d9955f361 100644
> --- a/rhel/openvswitch-dkms.spec.in
> +++ b/rhel/openvswitch-dkms.spec.in
> @@ -27,6 +27,8 @@ BuildRoot:%(mktemp -ud 
> %{_tmppath}/%{name}-%{version}-%{release}-XX)
>  # conflicts with the openvswitch-debuginfo package for OVS userspace).
>  %undefine _enable_debug_packages
> 
> +# Exclude input files from mangling
> +%global __brp_mangle_shebangs_exclude_from ^/usr/src/.*$
> 
>  %description
>  Open vSwitch Linux kernel module.
> --
> 2.30.2

Hi,
since almost any file is under /usr/src, you could just disable the
mangling feature entirely [1] instead of specify a regexp:

%undefine __brp_mangle_shebangs

[1]
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_shebang_lines

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

Re: [ovs-dev] [PATCH ovn] ofctrl: Don't assert while merging tracked flows.

[Han Zhou]

On Thu, Apr 29, 2021 at 9:25 AM Numan Siddique  wrote:
>
> On Wed, Apr 28, 2021 at 3:10 PM Han Zhou  wrote:
> >
> > On Wed, Apr 28, 2021 at 3:34 AM Numan Siddique  wrote:
> > >
> > > On Wed, Apr 28, 2021 at 3:31 AM Han Zhou  wrote:
> > > >
> > > > Thanks Numan for working on this issue!
> > > >
> > > > On Mon, Apr 26, 2021 at 3:53 AM  wrote:
> > > > >
> > > > > From: Numan Siddique 
> > > > >
> > > > > Presently we do ovs_assert(del_f->installed_flows) in
> > > > > merged_tracked_flows() of ofctrl.c if 'del_f' and flow 'f' are
equal
> > > > > and if 'del_f' is not installed.  But there are a couple of
scenarios
> > > > > this can happen:
> > > > >
> > > > > 1. A flow 'F1' was added to the desired flows, but ofctrl_put()
> > couldn't
> > > > > install the flow (because of the rconn queue is full) and an SB
update
> > > > > caused 'F1' to be removed and re-added as 'F2'.
> > > > >
> > > >
> > > > This seems not possible, because del_f->installed_flow is set
whenever a
> > > > "installed_flow" is added to the installed_flows table, regardless
of
> > > > whether it is sent to OVS or not.
> > >
> > > This can happen if ofctrl_can_put() returns false in which case
> > > ovn-controller will
> > > not call ofctrl_put() at all. There is a theoretical  possibility here
> > for sure.
> > >
> > > Let me know if you think otherwise.
> > >
> >
> > Oh, sorry that I misunderstood. You are right that ofctrl_put() may
return
> > immediately without installing the desired flow F1, but if that happens,
> > F1->installed_flow should be NULL, and then it is the same case as in
> > scenario 2) below: when F1 is deleted, it would be destroyed in
> > track_flow_del(). So we should never end up with a tracked deleted flow
> > with installed_flow being NULL, right? The logic is, in theory, when a
> > "desired but not yet installed" flow is being deleted, we should simply
> > destroy it and remove it from tracking. Maybe we should check if there
are
> > other possibilities.
>
> You're right.  F1 should be destroyed in track_flow_del().
>
> I got access to the core dump and I can see below details of "f" and
"del_f".
>
> Please let me know if this rings any bell
>
> -
> (gdb) frame 6
> #6  0x5569459bfbaa in merge_tracked_flows
> (flow_table=0x5569470c5b40) at
>
/usr/src/debug/ovn2.13-20.12.0-24.el8fdp.x86_64/openvswitch-2.14.90/include/openvswitch/hmap.h:283
> 283 hmap_expand_at(hmap, where);
> (gdb) print del_f
> $1 = (struct desired_flow *) 0x556948c670b0
> (gdb) print *del_f
> $2 = {flow = {table_id = 37 '%', priority = 100, match = {{{flow =
> 0x55694909a540, mask = 0x55694909a560}, flows = {0x55694909a540,
> 0x55694909a560}}, tun_md = 0x0}, hash = 2338012780, ofpacts =
> 0x556948d96460, ofpacts_len = 432, cookie = 1418516517},
>   match_hmap_node = {hash = 2338012780, next = 0x0}, list_node = {prev
> = 0x556948c67100, next = 0x556948c67100}, references = {prev =
> 0x556948c67110, next = 0x556948c67110}, installed_flow = 0x0,
> installed_ref_list_node = {prev = 0x5569495bcd90,
> next = 0x5569495bcd90}, track_list_node = {prev = 0x5569470c5b88,
> next = 0x556948d2bfc8}, is_deleted = true}
>
Looking at the content of the del_f, the installed_ref_list_node is not
pointing to the field of the flow itself, which means at least it was in
some installed_flow's desired_refs list, which means it was installed at
least at some point before. Now that the "installed_flow" is NULL, it is
possible that the desired flow was installed but then unlinked from the
installed flow. But by reviewing the code I didn't find any path that
"unlink_installed_to_desired()" or "replace_installed_to_desired()" could
happen to this flow before this point. If it is unlinked it should also be
removed from the track list and also from the deleted_flows hmap and
destroyed so it should never be found again. Of course it is also possible
that this desired_flow data structure is corrupted so the fields
nstalled_ref_list_node and installed_flow just have inconsistent data. I
will keep looking into the code. At the same time, is this reproducible? We
could add some more logs to help debugging if this happens again.

>
> (gdb) print f
> $7 = (struct desired_flow *) 0x556948d2bf40
>
> (gdb) print *f
> $6 = {flow = {table_id = 37 '%', priority = 100, match = {{{flow =
> 0x556948eaf1a0, mask = 0x556948eaf1c0}, flows = {0x556948eaf1a0,
> 0x556948eaf1c0}}, tun_md = 0x0}, hash = 2338012780, ofpacts =
> 0x5569472a15e0, ofpacts_len = 432, cookie = 1418516517},
>   match_hmap_node = {hash = 2338012780, next = 0x556948da8070},
> list_node = {prev = 0x556948d2bf90, next = 0x556948d2bf90}, references
> = {prev = 0x556947b35980, next = 0x556947b35980}, installed_flow =
> 0x0, installed_ref_list_node = {prev = 0x556948d2bfb8,
> next = 0x556948d2bfb8}, track_list_node = {prev = 0x556948c67138,
> next = 0x556948e68658}, is_deleted = false}
>
> --
>
> Looks like there are some issues with physical flow handling.  Seems
> to me an installed flow F1 is