Re: [ovs-dev] [PATCH] dpif-netdev: ct maxconns config persistence

[lic121]

On Mon, Apr 25, 2022 at 08:47:32AM -0400, Aaron Conole wrote:
> lic121  writes:
> 
> > Max allowed conntrack entries is configurable with
> > 'ovs-appctl dpctl/ct-set-maxconns' command. In real scenarios,
> > this configuration is expected to survive from host reboot.
> >
> > Signed-off-by: lic121 
> > ---
> 
> One complication is that there are 2 other conntrack implementations
> that OvS supports - one for the Windows Netlink, and the other for
> Linux kernel netlink.  How do you deal with this parameter there?

As linux kernel datapath leverages the kernel implementent of conntrack.
Besides ovs, the kernel conntrack is used by kernel network stack
itself. A proper way to set linux kernel datapath conntrack max conns
could be sysctl interface (nf_conntrack_max).
But I am not sure if a similar system level configuration exists for windows
datapath.

> 
> Something in the datapath for this needs to accommodate these.  Maybe
> the DB should store the datapath name as well - that way each dp can
> lookup the configuration if supported (and if not we can either log the
> error, etc).  That does start to look a bit unfriendly, but at least it
> prevents user confusion with this knob.

Agree, to indiates the datapath name in configuration item name is
better than explanation in configuration detail description.
How about the name "userspace-ct-maxconns"?

> 
> Each CT dp interface does something slightly differently for
> configuration, and I'm not sure this knob as proposed is the best
> solution.
> 
> The 'deprecated' command was only implemented for the userspace DP, but
> in theory it could work independently for all.  I'm not sure this one
> can work though (for example, if we use AF_XDP for some bridge, but
> kernel DP for another, we would want to have two different max entry
> values, but this interface doesn't allow that).
> 
> >  lib/dpctl.man   |  3 ++-
> >  lib/dpif-netdev.c   | 10 ++
> >  tests/system-traffic.at | 10 ++
> >  vswitchd/vswitch.xml|  7 +++
> >  4 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/dpctl.man b/lib/dpctl.man
> > index c100d0a..2cfc4f2 100644
> > --- a/lib/dpctl.man
> > +++ b/lib/dpctl.man
> > @@ -343,7 +343,8 @@ system due to connection tracking or simply limiting 
> > connection
> >  tracking.  If the number of connections is already over the new maximum
> >  limit request then the new maximum limit will be enforced when the
> >  number of connections decreases to that limit, which normally happens
> > -due to connection expiry.  Only supported for userspace datapath.
> > +due to connection expiry.  Only supported for userspace datapath. This
> > +command is deprecated by ovsdb cfg other_config:ct-maxconns.
> >  .
> >  .TP
> >  \*(DX\fBct\-get\-maxconns\fR [\fIdp\fR]
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 6764343..c518d30 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -4827,6 +4827,16 @@ dpif_netdev_set_config(struct dpif *dpif, const 
> > struct smap *other_config)
> >  }
> >  }
> >  
> > +uint32_t ct_maxconns, cur_maxconns;
> > +ct_maxconns = smap_get_int(other_config, "ct-maxconns", UINT32_MAX);
> > +/* Leave runtime value as it is when cfg is removed. */
> > +if (ct_maxconns < UINT32_MAX) {
> > +conntrack_get_maxconns(dp->conntrack, _maxconns);
> > +if (ct_maxconns != cur_maxconns) {
> > +conntrack_set_maxconns(dp->conntrack, ct_maxconns);
> > +}
> > +}
> > +
> >  bool smc_enable = smap_get_bool(other_config, "smc-enable", false);
> >  bool cur_smc;
> >  atomic_read_relaxed(>smc_enable_db, _smc);
> > diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> > index 4a7fa49..00fefc5 100644
> > --- a/tests/system-traffic.at
> > +++ b/tests/system-traffic.at
> > @@ -2258,6 +2258,16 @@ AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
> >  10
> >  ])
> >  
> > +AT_CHECK([ovs-vsctl set Open_vswitch . other_config:ct-maxconns=20], [0])
> > +AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
> > +20
> > +])
> > +
> > +AT_CHECK([ovs-vsctl remove Open_vswitch . other_config ct-maxconns], [0])
> > +AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
> > +20
> > +])
> > +
> >  OVS_TRAFFIC_VSWITCHD_STOP
> >  AT_CLEANUP
> >  
> > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> > index 0c66326..ec634d5 100644
> > --- a/vswitchd/vswitch.xml
> > +++ b/vswitchd/vswitch.xml
> > @@ -183,6 +183,13 @@
> >  
> >
> >  
> > +   > +  type='{"type": "integer", "minInteger": 0}'>
> > +The maximum number of connection tracker entries allowed in the
> > +datapath.  Only supported for userspace datapath.  This deprecates
> > +"ovs-appctl dpctl/ct-set-maxconns" command.
> > +  
> > +
> > >type='{"type": "integer", "minInteger": 500}'>
> >  
> 
___
dev 

Re: [ovs-dev] [PATCH v4] ofproto-dpif-xlate: Clear out vlan flow fields while processing native tunnel.

[Ilya Maximets]

On 4/27/22 08:59, Thilak Raj Surendra Babu wrote:
> When a packet is received over an access port that needs to be sent
> over a vxlan tunnel,the access port VLAN id is used in the lookup
> leading to a wrong packet being crafted and sent over the tunnel.
> Clear out the flow 's VLAN field as it should not be used while
> performing mac lookup for the outer tunnel and also at this point
> the VLAN action related to inner flow is already committed.
> 
> Fixes: 7c12dfc527a5 ("tunneling: Avoid datapath-recirc by combining recirc 
> actions at xlate.")
> Signed-off-by: Thilak Raj Surendra Babu 
> Signed-off-by: Rosemarie O'Riorden 
> Co-authored-by: Rosemarie O'Riorden 
> 
> ---
> Version 4:
>   - Address minor review comments.
>   - Replace with a simpler system test.
> 
> Version 3:
>   - Address review comments.
>   - Add unit-test patch from Rosemarie O'Riorden.
> 
> Version 2:
>   - Fixed line length warning from checkpatch.
>   - Dropped unrelated changes.
> 
>  ofproto/ofproto-dpif-xlate.c |  3 +++
>  tests/system-traffic.at  | 47 
>  tests/tunnel-push-pop.at | 52 
>  3 files changed, 102 insertions(+)

Thanks!  Applied and backported down to 2.13.

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

Re: [ovs-dev] [PATCH] dpif-netdev-avx512: Fix overflow of UINT32_C(1).

[Ilya Maximets]

On 4/27/22 19:35, Ilya Maximets wrote:
> On 4/27/22 18:41, Van Haaren, Harry wrote:
>>> -Original Message-
>>> From: Ferriter, Cian 
>>> Sent: Wednesday, April 27, 2022 4:58 PM
>>> To: ovs-dev@openvswitch.org
>>> Cc: echau...@redhat.com; i.maxim...@ovn.org; Van Haaren, Harry
>>> ; david.march...@redhat.com; Ferriter, Cian
>>> 
>>> Subject: [PATCH] dpif-netdev-avx512: Fix overflow of UINT32_C(1).
>>>
>>> UINT64_C(1) is required in this bitshift since batch_size can be 32 and
>>> 1 << 32 overflows UINT32_C(1).
>>>
>>> Fixes: ba0a2619ca0c ("dpif-netdev-avx512: Fix ubsan shift error in 
>>> bitmasks.")
>>> Signed-off-by: Cian Ferriter 
>>
>> Ugh, unsigned shift is defined, so UBSan won't flag *this* shift-out issue,
>> but will flag signed int overflow. Thanks Cian for finding reporting & 
>> fixing!
> 
> Hmm.  "If the value of the right operand is negative or is greater
> than or equal to the width of the promoted left operand, the behavior
> is undefined." according to the standard.  And UBsan does flag this
> construction for me if I'm putting it into a different place in code
> (I don't have avx512 machine at the moment):
> 
>   runtime error: shift exponent 32 is too large for 32-bit type 'unsigned int'
> 
> AFAICT, on Intel CPU this will also result in a bitmask being zero,
> causing raw_ctz(0) (__builtin_ctz(0)) call which is also undefined
> according to compiler docs, but it seem to return 32 as a result.
> Next the subsequent invalid memory access at packets[32] that Asan
> should be able to catch.  And OVS should also crash, because it highly
> unlikely for packets[32] to be a valid pointer.
> 
>>
>> Acked-by: Harry van Haaren 

Applied to master and branch-2.17.  Thanks!

Best regards, Ilya Maximets.

>>
>>> ---
>>> The other uses of UINT32_C(1) in the dpif-netdev-avx512 files are valid
>>> since those shifts are a maximum of 31 in all cases.
>>
>> Agreed, thanks for the explaining note.
>>
>> 
> 

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

Re: [ovs-dev] OVS DPDK DMA-Dev library/Design Discussion

[Bruce Richardson]

On Mon, Apr 25, 2022 at 11:46:01PM +0200, Ilya Maximets wrote:
> On 4/20/22 18:41, Mcnamara, John wrote:
> >> -Original Message-
> >> From: Ilya Maximets 
> >> Sent: Friday, April 8, 2022 10:58 AM
> >> To: Hu, Jiayu ; Maxime Coquelin
> >> ; Van Haaren, Harry
> >> ; Morten Brørup ;
> >> Richardson, Bruce 
> >> Cc: i.maxim...@ovn.org; Pai G, Sunil ; Stokes, Ian
> >> ; Ferriter, Cian ; ovs-
> >> d...@openvswitch.org; d...@dpdk.org; Mcnamara, John
> >> ; O'Driscoll, Tim ;
> >> Finn, Emma 
> >> Subject: Re: OVS DPDK DMA-Dev library/Design Discussion
> >>
> >> On 4/8/22 09:13, Hu, Jiayu wrote:
> >>>
> >>>
>  -Original Message-
>  From: Ilya Maximets 
>  Sent: Thursday, April 7, 2022 10:40 PM
>  To: Maxime Coquelin ; Van Haaren, Harry
>  ; Morten Brørup
>  ; Richardson, Bruce
>  
>  Cc: i.maxim...@ovn.org; Pai G, Sunil ; Stokes,
>  Ian ; Hu, Jiayu ; Ferriter,
>  Cian ; ovs-dev@openvswitch.org;
>  d...@dpdk.org; Mcnamara, John ; O'Driscoll,
>  Tim ; Finn, Emma 
>  Subject: Re: OVS DPDK DMA-Dev library/Design Discussion
> 
>  On 4/7/22 16:25, Maxime Coquelin wrote:
> > Hi Harry,
> >
> > On 4/7/22 16:04, Van Haaren, Harry wrote:
> >> Hi OVS & DPDK, Maintainers & Community,
> >>
> >> Top posting overview of discussion as replies to thread become
> >> slower:
> >> perhaps it is a good time to review and plan for next steps?
> >>
> >>  From my perspective, it those most vocal in the thread seem to be
> >> in favour of the clean rx/tx split ("defer work"), with the
> >> tradeoff that the application must be aware of handling the async
> >> DMA completions. If there are any concerns opposing upstreaming of
> >> this
>  method, please indicate this promptly, and we can continue technical
>  discussions here now.
> >
> > Wasn't there some discussions about handling the Virtio completions
> > with the DMA engine? With that, we wouldn't need the deferral of work.
> 
>  +1
> 
>  With the virtio completions handled by DMA itself, the vhost port
>  turns almost into a real HW NIC.  With that we will not need any
>  extra manipulations from the OVS side, i.e. no need to defer any work
>  while maintaining clear split between rx and tx operations.
> >>>
> >>> First, making DMA do 2B copy would sacrifice performance, and I think
> >>> we all agree on that.
> >>
> >> I do not agree with that.  Yes, 2B copy by DMA will likely be slower than
> >> done by CPU, however CPU is going away for dozens or even hundreds of
> >> thousands of cycles to process a new packet batch or service other ports,
> >> hence DMA will likely complete the transmission faster than waiting for
> >> the CPU thread to come back to that task.  In any case, this has to be
> >> tested.
> >>
> >>> Second, this method comes with an issue of ordering.
> >>> For example, PMD thread0 enqueue 10 packets to vring0 first, then PMD
> >>> thread1 enqueue 20 packets to vring0. If PMD thread0 and threa1 have
> >>> own dedicated DMA device dma0 and dma1, flag/index update for the
> >>> first 10 packets is done by dma0, and flag/index update for the left
> >>> 20 packets is done by dma1. But there is no ordering guarantee among
> >>> different DMA devices, so flag/index update may error. If PMD threads
> >>> don't have dedicated DMA devices, which means DMA devices are shared
> >>> among threads, we need lock and pay for lock contention in data-path.
> >>> Or we can allocate DMA devices for vring dynamically to avoid DMA
> >>> sharing among threads. But what's the overhead of allocation mechanism?
> >> Who does it? Any thoughts?
> >>
> >> 1. DMA completion was discussed in context of per-queue allocation, so
> >> there
> >>is no re-ordering in this case.
> >>
> >> 2. Overhead can be minimal if allocated device can stick to the queue for
> >> a
> >>reasonable amount of time without re-allocation on every send.  You may
> >>look at XPS implementation in lib/dpif-netdev.c in OVS for example of
> >>such mechanism.  For sure it can not be the same, but ideas can be re-
> >> used.
> >>
> >> 3. Locking doesn't mean contention if resources are allocated/distributed
> >>thoughtfully.
> >>
> >> 4. Allocation can be done be either OVS or vhost library itself, I'd vote
> >>for doing that inside the vhost library, so any DPDK application and
> >>vhost ethdev can use it without re-inventing from scratch.  It also
> >> should
> >>be simpler from the API point of view if allocation and usage are in
> >>the same place.  But I don't have a strong opinion here as for now,
> >> since
> >>no real code examples exist, so it's hard to evaluate how they could
> >> look
> >>like.
> >>
> >> But I feel like we're starting to run in circles here as I did already say
> >> most of that before.
> > 
> > 
> 
> Hi, John.
> 
> Just reading this email as I was on PTO for a last 1.5 weeks
> and 

Re: [ovs-dev] [PATCH] dpif-netdev-avx512: Fix overflow of UINT32_C(1).

[Ilya Maximets]

On 4/27/22 18:41, Van Haaren, Harry wrote:
>> -Original Message-
>> From: Ferriter, Cian 
>> Sent: Wednesday, April 27, 2022 4:58 PM
>> To: ovs-dev@openvswitch.org
>> Cc: echau...@redhat.com; i.maxim...@ovn.org; Van Haaren, Harry
>> ; david.march...@redhat.com; Ferriter, Cian
>> 
>> Subject: [PATCH] dpif-netdev-avx512: Fix overflow of UINT32_C(1).
>>
>> UINT64_C(1) is required in this bitshift since batch_size can be 32 and
>> 1 << 32 overflows UINT32_C(1).
>>
>> Fixes: ba0a2619ca0c ("dpif-netdev-avx512: Fix ubsan shift error in 
>> bitmasks.")
>> Signed-off-by: Cian Ferriter 
> 
> Ugh, unsigned shift is defined, so UBSan won't flag *this* shift-out issue,
> but will flag signed int overflow. Thanks Cian for finding reporting & fixing!

Hmm.  "If the value of the right operand is negative or is greater
than or equal to the width of the promoted left operand, the behavior
is undefined." according to the standard.  And UBsan does flag this
construction for me if I'm putting it into a different place in code
(I don't have avx512 machine at the moment):

  runtime error: shift exponent 32 is too large for 32-bit type 'unsigned int'

AFAICT, on Intel CPU this will also result in a bitmask being zero,
causing raw_ctz(0) (__builtin_ctz(0)) call which is also undefined
according to compiler docs, but it seem to return 32 as a result.
Next the subsequent invalid memory access at packets[32] that Asan
should be able to catch.  And OVS should also crash, because it highly
unlikely for packets[32] to be a valid pointer.

> 
> Acked-by: Harry van Haaren 
> 
>> ---
>> The other uses of UINT32_C(1) in the dpif-netdev-avx512 files are valid
>> since those shifts are a maximum of 31 in all cases.
> 
> Agreed, thanks for the explaining note.
> 
> 

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

Re: [ovs-dev] [PATCH] dpif-netdev-avx512: Fix overflow of UINT32_C(1).

[Van Haaren, Harry]

> -Original Message-
> From: Ferriter, Cian 
> Sent: Wednesday, April 27, 2022 4:58 PM
> To: ovs-dev@openvswitch.org
> Cc: echau...@redhat.com; i.maxim...@ovn.org; Van Haaren, Harry
> ; david.march...@redhat.com; Ferriter, Cian
> 
> Subject: [PATCH] dpif-netdev-avx512: Fix overflow of UINT32_C(1).
> 
> UINT64_C(1) is required in this bitshift since batch_size can be 32 and
> 1 << 32 overflows UINT32_C(1).
> 
> Fixes: ba0a2619ca0c ("dpif-netdev-avx512: Fix ubsan shift error in bitmasks.")
> Signed-off-by: Cian Ferriter 

Ugh, unsigned shift is defined, so UBSan won't flag *this* shift-out issue,
but will flag signed int overflow. Thanks Cian for finding reporting & fixing!

Acked-by: Harry van Haaren 

> ---
> The other uses of UINT32_C(1) in the dpif-netdev-avx512 files are valid
> since those shifts are a maximum of 31 in all cases.

Agreed, thanks for the explaining note.


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

[ovs-dev] [PATCH] dpif-netdev-avx512: Fix overflow of UINT32_C(1).

[Cian Ferriter]

UINT64_C(1) is required in this bitshift since batch_size can be 32 and
1 << 32 overflows UINT32_C(1).

Fixes: ba0a2619ca0c ("dpif-netdev-avx512: Fix ubsan shift error in bitmasks.")
Signed-off-by: Cian Ferriter 

---
The other uses of UINT32_C(1) in the dpif-netdev-avx512 files are valid
since those shifts are a maximum of 31 in all cases.
---
 lib/dpif-netdev-avx512.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
index 151a945a9..11d9a0005 100644
--- a/lib/dpif-netdev-avx512.c
+++ b/lib/dpif-netdev-avx512.c
@@ -159,7 +159,7 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread 
*pmd,
 mf_mask = mfex_func(packets, keys, batch_size, in_port, pmd);
 }
 
-uint32_t lookup_pkts_bitmask = (UINT32_C(1) << batch_size) - 1;
+uint32_t lookup_pkts_bitmask = (UINT64_C(1) << batch_size) - 1;
 uint32_t iter = lookup_pkts_bitmask;
 while (iter) {
 uint32_t i = raw_ctz(iter);
-- 
2.25.1

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

Re: [ovs-dev] [PATCH] tests: Add ovs-dpdk rate limiting unit tests.

[Stokes, Ian]

> > From: Seamus Ryan 
> >
> > This adds 4 new unit tests to the 'check-dpdk' subsystem that will
> > test rate limiting functionality.
> >
> 
> Thanks for the patch Michael, a few comments below.
> 
> I'm still conducting some testing so may have some follow up comments for full
> review.

Hi Michael, had some more time for testing today and have a few more follow up 
comments inline below for the v2.

> 
> > Signed-off-by: Seamus Ryan 
> Probably for the v2 you should add yourself as a Co-author and add your own
> sign off tag as well.
> 
> > ---
> >  NEWS |   3 +-
> >  tests/system-dpdk.at | 128
> > +++
> >  2 files changed, 130 insertions(+), 1 deletion(-)
> >
> > diff --git a/NEWS b/NEWS
> > index 8fa57836a..e1fbc39ed 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -3,7 +3,8 @@ Post-v2.17.0
> > - OVSDB:
> >   * 'relay' service model now supports transaction history, i.e. honors 
> > the
> > 'last-txn-id' field in 'monitor_cond_since' requests from clients.
> > -
> No need to remove the white space above, You can just add the DPDK section
> straight after the last line above.
> There should still be 2 white spaces between this section and the v2.17 
> section
> below.
> 
> > +   - DPDK:
> > + * Add rate limiting unit tests to DPDK testsuite
> Minor, missing period at end of the sentence above.
> 
> >
> >  v2.17.0 - 17 Feb 2022
> >  -
> > diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
> > index 7d2715c4a..6d4707f84 100644
> > --- a/tests/system-dpdk.at
> > +++ b/tests/system-dpdk.at
> > @@ -222,6 +222,134 @@ OVS_VSWITCHD_STOP("m4_join([],
> > [SYSTEM_DPDK_ALLOWED_LOGS], [
> >  AT_CLEANUP
> >  dnl 
> > --
> >
> Just to keep with the existing style for the majority of the unit tests please
> ensure 3 whitespaces between the dnl above and below here.
> 
> > +dnl 
> > --
> > +dn1 Rate create delete phy port
> Should this not be dnl instead of dn1?
> 
> As this is ingress policing I think it would be better to reflect that in the 
> text
> throughout, you can have rate limiting technically on the QoS (Egress) and on
> ingress policing also so it would be good to reflect which is being checked 
> here.
> 
> > +AT_SETUP([OVS-DPDK - Rate create delete phy port])
> > +AT_KEYWORDS([dpdk])
> > +
> > +OVS_DPDK_PRE_PHY_SKIP()
> > +OVS_DPDK_START()
> > +
> > +dnl Add userspace bridge and attach it to OVS and add policer
> > +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 datapath_type=netdev])
> > +AT_CHECK([ovs-vsctl add-port br10 phy0 -- set Interface phy0 type=dpdk
> > options:dpdk-devargs=$(cat PCI_ADDR)], [], [stdout], [stderr])
> > +AT_CHECK([ovs-vsctl set interface phy0 ingress_policing_rate=1
> > ingress_policing_burst=1000])
> > +AT_CHECK([ovs-vsctl show], [], [stdout])
> 
> What's the purpose of this check? I'm guessing that you are probably looking 
> to
> confirm the rate and burst levels but I don't see an explicit check for it?
> If so maybe you need to search for the required fields explicitly rather than 
> just
> the show command itself?

So just to confirm, show command here wont provide useful infor for the inress 
policer. However we could check the values for burst and rate with something 
like

ovs-vsctl get interface dpdk0 ingress_policing_burst
ovs-vsctl get interface dpdk0 ingress_policing_rate

To ensure the expected values are there for the given interface?
> 
> > +sleep 2
> > +
> > +dn1 remove policer
> dn1 should be dnl
> 
> > +AT_CHECK([ovs-vsctl set interface phy0 ingress_policing_rate=0
> > ingress_policing_burst=0])

Again we could confirm the rate/burst after it's set to 0 here to ensure it's 
after being set without error.

> > +
> > +dnl Clean up
> > +AT_CHECK([ovs-vsctl del-port br10 phy0], [], [stdout], [stderr])
> > +OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
> > +\@VHOST_CONFIG: failed to connect to $OVS_RUNDIR/dpdkvhostclient0:
> No
> > such file or directory@d
> > +])")
> Not sure why we need the OVS_VSWITCHD_STOP command above to check for
> vhost client connection error, this test only looks at using a standard DPDK 
> PHY
> device so we shouldn't see this message would be my thinking as we are not
> adding a vhost port?
> 
> > +AT_CLEANUP
> > +dnl 
> > --
> > +
> > +dnl 
> > --
> > +dn1 Rate create delete vport port
> 
> dn1 -> dnl
> 
> > +AT_SETUP([OVS-DPDK - Rate create delete vport port])
> > +AT_KEYWORDS([dpdk])
> > +
> > +OVS_DPDK_PRE_CHECK()
> > +OVS_DPDK_START()
> > +
> > +dnl Add userspace bridge and attach it to OVS and add policer
> > +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 datapath_type=netdev])
> > +AT_CHECK([ovs-vsctl add-port br10 

Re: [ovs-dev] [PATCH v4 2/2] ofp-monitor: Support flow monitoring for OpenFlow 1.3, 1.4+

[Aaron Conole]

Vasu Dasari  writes:

> Extended OpenFlow monitoring support
> * OpenFlow 1.3 with ONF extensions
> * OpenFlow 1.4+ as defined in OpenFlow specification 1.4+.
>
> ONF extensions are similar to Nicira extensions except for 
> onf_flow_monitor_request{}
> where out_port is defined as 32-bit number OF(1.1) number, oxm match formats 
> are
> used in update and request messages.
>
> Flow monitoring support in 1.4+ is slightly different from Nicira and ONF
> extensions.
>  * More flow monitoring flags are defined.
>  * Monitor add/modify/delete command is intruduced in flow_monitor
>request message.
>  * Addition of out_group as part of flow_monitor request message
>
> Description of changes:
> 1. Generate ofp-msgs.inc to be able to support 1.3, 1.4+ flow Monitoring 
> messages.
> include/openvswitch/ofp-msgs.h
>
> 2. Modify openflow header files with protocol specific headers.
> include/openflow/openflow-1.3.h
> include/openflow/openflow-1.4.h
>
> 3. Modify OvS abstraction of openflow headers. ofp-monitor.h leverages  enums
>from on nicira extensions for creating protocol abstraction headers. 
> OF(1.4+)
>enums are superset of nicira extensions.
> include/openvswitch/ofp-monitor.h
>
> 4. Changes to these files reflect encoding and decoding of new protocol 
> messages.
> lib/ofp-monitor.c
>
> 5. Changes to mmodules using ofp-monitor APIs. Most of the changes here are to
>migrate enums from nicira to OF 1.4+ versions.
> ofproto/connmgr.c
> ofproto/connmgr.h
> ofproto/ofproto-provider.h
> ofproto/ofproto.c
>
> 6. Extended protocol decoding tests to verify all protocol versions
> FLOW_MONITOR_CANCEL
> FLOW_MONITOR_PAUSED
> FLOW_MONITOR_RESUMED
> FLOW_MONITOR request
> FLOW_MONITOR reply
> tests/ofp-print.at
>
> 7. Modify flow monitoring tests to be able executed by all protocol versions.
> tests/ofproto.at
>
> 7. Modified documentation highlighting the change
> utilities/ovs-ofctl.8.in
> NEWS
>
> Signed-off-by: Vasu Dasari 
> Reported-at: 
> https://mail.openvswitch.org/pipermail/ovs-dev/2021-June/383915.html
> ---

Same as previous - looks good, but needs a respin due to NEWS

Acked-by: Aaron Conole 

> v1:
>  - Fixed 0-day Robot errors
> v4:
>  - Rebase code with latest master.
> ---
>  NEWS  |   6 +-
>  include/openflow/openflow-1.3.h   |  89 
>  include/openflow/openflow-1.4.h   |  93 +++-
>  include/openvswitch/ofp-monitor.h |   9 +-
>  include/openvswitch/ofp-msgs.h|  39 +-
>  lib/ofp-monitor.c | 844 --
>  lib/ofp-print.c   |  24 +-
>  ofproto/connmgr.c |  47 +-
>  ofproto/connmgr.h |   6 +-
>  ofproto/ofproto-provider.h|   4 +-
>  ofproto/ofproto.c |  89 +++-
>  tests/ofp-print.at| 122 -
>  tests/ofproto.at  | 176 +--
>  utilities/ovs-ofctl.8.in  |   3 +
>  utilities/ovs-ofctl.c |   6 +
>  15 files changed, 1265 insertions(+), 292 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index d8e6c06f7..0555cc8fb 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -47,8 +47,10 @@ v2.16.0 - 16 Aug 2021
> - In ovs-vsctl and vtep-ctl, the "find" command now accept new
>   operators {in} and {not-in}.
> - OpenFlow:
> - * Extend Flow Monitoring support for OpenFlow 1.0-1.2 with Nicira
> -   Extensions
> + * Extended Flow Monitoring support for all supported OpenFlow versions
> + OpenFlow versions 1.0-1.2 with Nicira Extensions
> + OpenFlow versions 1.3 with Open Network Foundation extension
> + OpenFlow versions 1.4+, as defined in the OpenFlow specification
> - Userspace datapath:
>   * Auto load balancing of PMDs now partially supports cross-NUMA polling
> cases, e.g if all PMD threads are running on the same NUMA node.
> diff --git a/include/openflow/openflow-1.3.h b/include/openflow/openflow-1.3.h
> index c48a8ea7f..1a818dbb4 100644
> --- a/include/openflow/openflow-1.3.h
> +++ b/include/openflow/openflow-1.3.h
> @@ -374,4 +374,93 @@ struct ofp13_async_config {
>  };
>  OFP_ASSERT(sizeof(struct ofp13_async_config) == 24);
>  
> +struct onf_flow_monitor_request {
> +ovs_be32   id;/* Controller-assigned ID for this monitor. */
> +ovs_be16   flags; /* ONFFMF_*. */
> +ovs_be16   match_len; /* Length of oxm_fields. */
> +ovs_be32   out_port;  /* Required output port, if not OFPP_NONE. */
> +uint8_ttable_id;  /* One table’s ID or 0xff for all tables. */
> +uint8_tzeros[3];  /* Align to 64 bits (must be zero). */
> +/* Followed by an ofp11_match structure. */
> +};
> +OFP_ASSERT(sizeof(struct onf_flow_monitor_request) == 16);
> +
> +/* Header for experimenter requests and replies. */
> +struct onf_experimenter_header {
> +struct ofp_header header;
> +ovs_be32   vendor;/* 

Re: [ovs-dev] [PATCH v4 1/2] ofp-monitor: Extend Flow Monitoring support for OF 1.0-1.2 with Nicira Extensions

[Aaron Conole]

Vasu Dasari  writes:

> Currently OVS supports flow-monitoring for OpenFlow 1.0 and Nicira 
> Extenstions.
> Any other OpenFlow versioned messages are not accepted. This checkin will 
> allow
> OpenFlow1.0-1.2 Flow Monitoring wth Nicira extensions be accepted. Also made
> sure that flow-monitoring updates, flow monitoring pause messages, resume
> messages are sent in the same OpenFlow version as that of flow-monitor 
> request.
>
> Description of changes:
>
> 1. Generate ofp-msgs.inc to be able to support 1.0-1.2 Flow Monitoring 
> messages.
> include/openvswitch/ofp-msgs.h
>
> 2. Support vconn to accept user specified version and use it for vconn
> flow-monitoring session
> ofproto/ofproto.c
>
> 3. Modify APIs to use protocol as an argument to encode and decode messages
> include/openvswitch/ofp-monitor.h
> lib/ofp-monitor.c
> ofproto/connmgr.c
> ofproto/connmgr.h
> ofproto/ofproto.c
>
> 4. Modified following testcases to be verified across supported OF Versions
> ofproto - flow monitoring
> ofproto - flow monitoring with !own
> ofproto - flow monitoring with out_port
> ofproto - flow monitoring pause and resume
> ofproto - flow monitoring usable protocols
> tests/ofproto.at
>
> 5. Updated NEWS with the support added with this commit
>
> Signed-off-by: Vasu Dasari 
> Reported-at: 
> https://mail.openvswitch.org/pipermail/ovs-discuss/2020-December/050820.html
> ---

Thanks for the work on this - sorry for the late review.

In general, it looks good.  It will need to be rebased for the NEWS
entries, otherwise it applies cleanly still.

Acked-by: Aaron Conole 

> v1:
>  - Addressed code review comments from Ben Pfaff:
>   1. Reduced supported versions from OF 1.0+ to 1.0-1.3 as there is flow
>  monitoring is supported in OF 1.4+. Need to make changes as part of 
> future
>  development to add support for OpenFlow 1.4+.
>   2. Added announcement of this support to NEWS.
>   3. Extended test cases identified in commit to be tested agains all 
> supported
>  OF versions.
> v2:
>  - Fix 0-day robot error in NEWS file.
> v3:
>  - Addressed code review comments.
>  - Reduced OF versions supported to (1.0-1.2).
>  - Made all flow monitoring tests to be tested against all openflow versions.
>  - Added an option to dump openflow packets in hex for 
> add-flow/del-flow/mod-flow.
>The option to dump bytes is available for "dump" commands but not for the 
> flow
>management commands. Using this option to be able to generate packets 
> dynamically
>during the flow monitoring tests.
> v4:
>  - Rebased code with latest master.
>  ---
> ---
>  AUTHORS.rst   |   2 +-
>  NEWS  |   3 +
>  include/openvswitch/ofp-monitor.h |   9 +-
>  include/openvswitch/ofp-msgs.h|   4 +-
>  lib/ofp-monitor.c |  20 ++--
>  ofproto/connmgr.c |  18 +++-
>  ofproto/connmgr.h |   3 +-
>  ofproto/ofproto.c |  13 ++-
>  tests/ofproto.at  | 172 +-
>  utilities/ovs-ofctl.c |  22 +++-
>  10 files changed, 160 insertions(+), 106 deletions(-)
>
> diff --git a/AUTHORS.rst b/AUTHORS.rst
> index 93a3c30ed..ef4ef996c 100644
> --- a/AUTHORS.rst
> +++ b/AUTHORS.rst
> @@ -420,6 +420,7 @@ Tony van der Peet  
> tony.vanderp...@alliedtelesis.co.nz
>  Tonghao Zhang  xiangxia.m@gmail.com
>  Usman Ansari   ua1...@gmail.com
>  Valient Gough  vgo...@pobox.com
> +Vasu Dasarivdas...@gmail.com
>  Venkata Anil Kommaddi  vkomm...@redhat.com
>  Vishal Deep Ajmera vishal.deep.ajm...@ericsson.com
>  Vivien Bernet-Rollande v...@soprive.net
> @@ -692,7 +693,6 @@ Tulio Ribeiro   
> tribe...@lasige.di.fc.ul.pt
>  Tytus Kurek tytus.ku...@pega.com
>  Valentin Budvalen...@hackaserver.com
>  Vasiliy Tolstov v.tols...@selfip.ru
> -Vasu Dasari vdas...@gmail.com
>  Vinllen Chencvinl...@gmail.com
>  Vishal Swarankarvishal.swarn...@gmail.com
>  Vjekoslav Brajkovic bal...@cs.washington.edu
> diff --git a/NEWS b/NEWS
> index f5bd79b5a..d8e6c06f7 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -46,6 +46,9 @@ v2.16.0 - 16 Aug 2021
> processing database updates.
> - In ovs-vsctl and vtep-ctl, the "find" command now accept new
>   operators {in} and {not-in}.
> +   - OpenFlow:
> + * Extend Flow Monitoring support for OpenFlow 1.0-1.2 with Nicira
> +   Extensions
> - Userspace datapath:
>   * Auto load balancing of PMDs now partially supports cross-NUMA polling
> cases, e.g if all PMD threads are running on the same NUMA node.
> diff --git a/include/openvswitch/ofp-monitor.h 
> b/include/openvswitch/ofp-monitor.h
> index 237cef85e..835efd0f3 100644
> --- 

Re: [ovs-dev] OVS DPDK DMA-Dev library/Design Discussion

[Mcnamara, John]

> -Original Message-
> From: Ilya Maximets 
> Sent: Monday, April 25, 2022 10:46 PM
> To: Mcnamara, John ; Hu, Jiayu
> ; Maxime Coquelin ; Van
> Haaren, Harry ; Morten Brørup
> ; Richardson, Bruce
> 
> Cc: i.maxim...@ovn.org; Pai G, Sunil ; Stokes,
> Ian ; Ferriter, Cian ;
> ovs-dev@openvswitch.org; d...@dpdk.org; O'Driscoll, Tim
> ; Finn, Emma 
> Subject: Re: OVS DPDK DMA-Dev library/Design Discussion
> 
> ...
> 
> FWIW, I think it makes sense to PoC and test options that are going to
> be simply unavailable going forward if not explored now.
> Especially because we don't have any good solutions anyway ("Deferral
> of Work" is architecturally wrong solution for OVS).

I agree that there is value in doing PoCs and we have been doing that for over 
a year based on different proposals and none of them show the potential of the 
Deferral of Work approach. It isn't productive to keep building PoCs 
indefinitely; at some point we need to make progress with merging a specific 
solution upstream.


> > Let's have another call so that we can move towards a single solution
> that the DPDK and OVS communities agree on. I'll set up a call for next
> week in a similar time slot to the previous one.
> 
> Is there any particular reason we can't use a mailing list to discuss
> that topic further?

The discussion can continue on the mailing list. It just seemed more efficient 
and interactive to discuss this in a meeting.

John
-- 


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

[ovs-dev] [PATCH ovn v3] controller/binding: prevent claiming container lport to ovs bridge

[Mohammad Heib]

currently ovn-controller allow users to claim lport of type container
to ovs bridge which is invalid use-case.

This patch will prevent such invalid use-cases by ignoring the claiming
requests for container lports and will throw a warning message to the
controller logs.

Signed-off-by: Mohammad Heib 
---
 controller/binding.c | 34 ++
 tests/ovn.at | 11 +++
 2 files changed, 45 insertions(+)

diff --git a/controller/binding.c b/controller/binding.c
index 7281b0485..db0a6f118 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -970,6 +970,32 @@ claim_lport(const struct sbrec_port_binding *pb,
 return true;
 }
 
+/* Validate that "iface_rec:external_ids:iface-id" belongs to a lport
+ * that can be claimed to OVS bridge.
+ *
+ * If the lport type is LP_CONTAINER this function will throw a warning
+ * message to the logs and return "false".
+ *
+ * Otherwise, return "true".
+ */
+static bool
+valid_iface_claim(const char *iface_id,
+  struct binding_ctx_in *b_ctx_in)
+{
+const struct sbrec_port_binding *pb = NULL;
+pb = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name,
+  iface_id);
+if (pb && (get_lport_type(pb) == LP_CONTAINER)) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
+VLOG_WARN_RL(, "Can't claim lport %s of type container to"
+ " OVS bridge,\nplease remove the lport parent_name"
+ " before claiming it.", pb->logical_port);
+return false;
+}
+
+return true;
+}
+
 /* Returns false if lport is not released due to 'sb_readonly'.
  * Returns true otherwise.
  *
@@ -1497,6 +1523,10 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in,
 struct local_binding *lbinding =
 local_binding_find(local_bindings, iface_id);
 if (!lbinding) {
+if (!valid_iface_claim(iface_id, b_ctx_in)) {
+continue;
+}
+
 lbinding = local_binding_create(iface_id, iface_rec);
 local_binding_add(local_bindings, lbinding);
 } else {
@@ -2022,6 +2052,10 @@ binding_handle_ovs_interface_changes(struct 
binding_ctx_in *b_ctx_in,
 int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0;
 if (iface_id && ofport > 0 &&
 is_iface_in_int_bridge(iface_rec, b_ctx_in->br_int)) {
+if (!valid_iface_claim(iface_id, b_ctx_in)) {
+continue;
+}
+
 handled = consider_iface_claim(iface_rec, iface_id, b_ctx_in,
b_ctx_out, qos_map_ptr);
 if (!handled) {
diff --git a/tests/ovn.at b/tests/ovn.at
index 34b6abfc0..bc59f3f13 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -28535,6 +28535,11 @@ check ovn-nbctl --wait=sb set logical_switch_port vm1 
parent_name=vm-cont1
 
 wait_for_ports_up
 
+# Try to claim container port to ovs
+check ovn-nbctl set logical_switch_port vm-cont2 parent_name=vm2
+check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=vm-cont2
+AT_CHECK([test 1 = `cat hv1/ovn-controller.log |grep -c "claim lport vm-cont2 
of type container"`])
+
 # Delete vm1, vm-cont1 and vm-cont2 and recreate again.
 check ovn-nbctl lsp-del vm1
 check ovn-nbctl lsp-del vm-cont1
@@ -28546,6 +28551,12 @@ check ovn-nbctl lsp-add ls vm-cont1 vm1 1
 check ovn-nbctl lsp-add ls vm-cont2 vm1 2
 
 wait_for_ports_up
+check as hv1 ovn-appctl -t ovn-controller debug/pause
+# Try to claim container port to ovs with recompute
+check ovn-nbctl set logical_switch_port vm-cont2 parent_name=vm2
+check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=vm-cont2
+check as hv1 ovn-appctl -t ovn-controller debug/resume
+AT_CHECK([test 1 = `cat hv1/ovn-controller.log |grep -c "claim lport vm-cont2 
of type container"`])
 
 # Make vm1 as a child port of some non existent lport - foo. vm1, vm1-cont1 and
 # vm1-cont2 should be released.
-- 
2.34.1

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

[ovs-dev] User space connection tracking benchmarks

[Robin Jarry]

Hi all,

I have been working on some benchmarks of user space connection
tracking. I wanted to give some feedback on the results so that I got on
two patch series that were submitted by Paolo and Gaëtan a while ago.

In this intent, I have written a small script that makes use of T-Rex
ASTF API to generate arbitrary TCP and UDP connections:

https://github.com/rh-nfv-int/trex-core/blob/master/scripts/conntrack_ndr.py

TL;DR: Both series show significant improvement in conntrack creation
and destruction for a single core (around +240%). However, there seem to
be scalability issues with multiple cores in Gaëtan's series.

Detailed benchmark procedure and results:
https://gist.github.com/rjarry/efe91f14a9bda4eb287592f696dbd123

Gaëtan Rivet's series:
https://patchwork.ozlabs.org/project/openvswitch/list/?series=292039

Paolo Valerio's series (first 3 patches are missing from the web ui):
https://patchwork.ozlabs.org/project/openvswitch/list/?series=291239

Cheers,

-- 
Robin

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

Re: [ovs-dev] [PATCH ovn v2 2/2] qos: add support for port minimum bandwidth guarantee

[Ales Musil]

Looks good, thanks

Acked-by: Ales Musil 

On Wed, Apr 27, 2022 at 2:03 AM Ihar Hrachyshka  wrote:

> Reported-At: https://bugzilla.redhat.com/show_bug.cgi?id=2060310
> Signed-off-by: Ihar Hrachyshka 
> ---
>  NEWS |  2 ++
>  controller/binding.c | 13 ++---
>  ovn-nb.xml   |  5 +
>  ovn-sb.xml   |  5 +
>  tests/system-ovn.at  |  4 +++-
>  5 files changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index dbe89e9cf..244824e3f 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -7,6 +7,8 @@ Post v22.03.0
>- Support NAT for logical routers with multiple distributed gateway
> ports.
>- Add global option (NB_Global.options:default_acl_drop) to enable
>  implicit drop behavior on logical switches with ACLs applied.
> +  - Support (LSP.options:qos_min_rate) to guarantee minimal bandwidth
> available
> +for a logical port.
>
>  OVN v22.03.0 - 11 Mar 2022
>  --
> diff --git a/controller/binding.c b/controller/binding.c
> index 7281b0485..0f08281ab 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -51,6 +51,7 @@ VLOG_DEFINE_THIS_MODULE(binding);
>  struct qos_queue {
>  struct hmap_node node;
>  uint32_t queue_id;
> +uint32_t min_rate;
>  uint32_t max_rate;
>  uint32_t burst;
>  };
> @@ -88,17 +89,19 @@ static void update_lport_tracking(const struct
> sbrec_port_binding *pb,
>  static void
>  get_qos_params(const struct sbrec_port_binding *pb, struct hmap
> *queue_map)
>  {
> +uint32_t min_rate = smap_get_int(>options, "qos_min_rate", 0);
>  uint32_t max_rate = smap_get_int(>options, "qos_max_rate", 0);
>  uint32_t burst = smap_get_int(>options, "qos_burst", 0);
>  uint32_t queue_id = smap_get_int(>options, "qdisc_queue_id", 0);
>
> -if ((!max_rate && !burst) || !queue_id) {
> +if ((!min_rate && !max_rate && !burst) || !queue_id) {
>  /* Qos is not configured for this port. */
>  return;
>  }
>
>  struct qos_queue *node = xzalloc(sizeof *node);
>  hmap_insert(queue_map, >node, hash_int(queue_id, 0));
> +node->min_rate = min_rate;
>  node->max_rate = max_rate;
>  node->burst = burst;
>  node->queue_id = queue_id;
> @@ -238,9 +241,12 @@ setup_qos(const char *egress_iface, struct hmap
> *queue_map)
>  HMAP_FOR_EACH_WITH_HASH (sb_info, node, hash_int(queue_id, 0),
>   queue_map) {
>  is_queue_needed = true;
> -if (sb_info->max_rate ==
> +if (sb_info->min_rate ==
> +smap_get_int(_details, "min-rate", 0)
> +&& sb_info->max_rate ==
>  smap_get_int(_details, "max-rate", 0)
> -&& sb_info->burst == smap_get_int(_details,
> "burst", 0)) {
> +&& sb_info->burst ==
> +smap_get_int(_details, "burst", 0)) {
>  /* This queue is consistent. */
>  hmap_insert(_queues, _info->node,
>  hash_int(queue_id, 0));
> @@ -265,6 +271,7 @@ setup_qos(const char *egress_iface, struct hmap
> *queue_map)
>  }
>
>  smap_clear(_details);
> +smap_add_format(_details, "min-rate", "%d",
> sb_info->min_rate);
>  smap_add_format(_details, "max-rate", "%d",
> sb_info->max_rate);
>  smap_add_format(_details, "burst", "%d", sb_info->burst);
>  error = netdev_set_queue(netdev_phy, sb_info->queue_id,
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 9010240a8..626ddf266 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -1036,6 +1036,11 @@
> table.
>  
>
> +
> +  If set, indicates the minimum guaranteed rate available for
> data sent
> +  from this interface, in bit/s.
> +
> +
>  
>If set, indicates the maximum rate for data sent from this
> interface,
>in bit/s. The traffic will be shaped according to this limit.
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index 1ffb24e7a..b0b11b8ee 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -3254,6 +3254,11 @@ tcp.flags = RST;
>   table.
>
>
> +  
> +If set, indicates the minimum guaranteed rate available for data
> sent
> +from this interface, in bit/s.
> +  
> +
>
>  If set, indicates the maximum rate for data sent from this
> interface,
>  in bit/s. The traffic will be shaped according to this limit.
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 1454f99d2..4bf22593a 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -6338,13 +6338,15 @@ ovn-nbctl lsp-add sw0 public \
>  -- lsp-set-type public localnet \
>  -- lsp-set-options public network_name=phynet
>
> +AT_CHECK([ovn-nbctl set Logical_Switch_Port public
> options:qos_min_rate=20])
>  AT_CHECK([ovn-nbctl set Logical_Switch_Port public
> options:qos_max_rate=30])
>  AT_CHECK([ovn-nbctl set 

Re: [ovs-dev] [PATCH ovn v2 1/2] tests: check qos_max_rate and qos_burst are set

[Ales Musil]

Looks good, thanks

Acked-by: Ales Musil 

On Wed, Apr 27, 2022 at 2:03 AM Ihar Hrachyshka  wrote:

> Signed-off-by: Ihar Hrachyshka 
> ---
>  tests/system-ovn.at | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 3d2591ee9..1454f99d2 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -6338,11 +6338,15 @@ ovn-nbctl lsp-add sw0 public \
>  -- lsp-set-type public localnet \
>  -- lsp-set-options public network_name=phynet
>
> -AT_CHECK([ovn-nbctl set Logical_Switch_Port public
> options:qos_burst=1000])
> +AT_CHECK([ovn-nbctl set Logical_Switch_Port public
> options:qos_max_rate=30])
> +AT_CHECK([ovn-nbctl set Logical_Switch_Port public
> options:qos_burst=300])
>  AT_CHECK([ovs-vsctl set interface ovs-public
> external-ids:ovn-egress-iface=true])
>  OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-public'])
> +OVS_WAIT_UNTIL([tc class show dev ovs-public | \
> +grep -q 'class htb .* ceil 300Kbit burst 375000b cburst
> 375000b'])
>
> -AT_CHECK([ovn-nbctl remove Logical_Switch_Port public options
> qos_burst=1000])
> +AT_CHECK([ovn-nbctl remove Logical_Switch_Port public options
> qos_max_rate=30])
> +AT_CHECK([ovn-nbctl remove Logical_Switch_Port public options
> qos_burst=300])
>  OVS_WAIT_UNTIL([test "$(tc qdisc show | grep 'htb 1: dev ovs-public')" =
> ""])
>
>  kill $(pidof ovn-controller)
> --
> 2.34.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>

-- 

Ales Musil

Senior Software Engineer - RHV Network

Red Hat EMEA 

amu...@redhat.comIM: amusil

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

[ovs-dev] [PATCH v3 3/3] flow: Add autovalidator support to miniflow_extract.

[Kumar Amber]

The patch adds the flag based switch between choice of using
miniflow_extract in normal pipeline or select mfex_autovalidator
in debug and test builds.

The compile time flag used to select autoval can be done using option:

 ./configure CFLAGS="--enable-mfex-default-autovalidator"

Signed-off-by: Kumar Amber 

---
v3:
- Fix comments from Cian.
---
---
 lib/dpif-netdev-private-extract.c |  8 
 lib/flow.c| 34 ++-
 lib/flow.h|  4 
 3 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/lib/dpif-netdev-private-extract.c 
b/lib/dpif-netdev-private-extract.c
index 42b970e75..bbc0e3c78 100644
--- a/lib/dpif-netdev-private-extract.c
+++ b/lib/dpif-netdev-private-extract.c
@@ -124,8 +124,8 @@ dpif_miniflow_extract_init(void)
 /* For the first call, this will be choosen based on the
  * compile time flag.
  */
-VLOG_INFO("Default MFEX Extract implementation is %s.\n",
-  mfex_impls[mfex_idx].name);
+VLOG_DBG("Default MFEX Extract implementation is %s.\n",
+ mfex_impls[mfex_idx].name);
 atomic_store_relaxed(mfex_func, (uintptr_t) mfex_impls
  [mfex_idx].extract_func);
 }
@@ -251,7 +251,7 @@ dpif_miniflow_extract_autovalidator(struct dp_packet_batch 
*packets,
 /* Run scalar miniflow_extract to get default result. */
 DP_PACKET_BATCH_FOR_EACH (i, packet, packets) {
 pkt_metadata_init(>md, in_port);
-miniflow_extract(packet, [i]);
+miniflow_extract_(packet, [i]);
 
 /* Store known good metadata to compare with optimized metadata. */
 good_l2_5_ofs[i] = packet->l2_5_ofs;
@@ -347,7 +347,7 @@ dpif_miniflow_extract_autovalidator(struct dp_packet_batch 
*packets,
 }
 
 /* Having dumped the debug info for the batch, disable autovalidator. */
-if (batch_failed) {
+if (batch_failed && (pmd != NULL)) {
 atomic_store_relaxed(>miniflow_extract_opt, NULL);
 }
 
diff --git a/lib/flow.c b/lib/flow.c
index 086096d5e..16698cedd 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -36,6 +36,8 @@
 #include "openvswitch/match.h"
 #include "dp-packet.h"
 #include "dpif-netdev-private-dpcls.h"
+#include "dpif-netdev-private-dpif.h"
+#include "dpif-netdev-private-extract.h"
 #include "openflow/openflow.h"
 #include "packets.h"
 #include "odp-util.h"
@@ -757,7 +759,7 @@ dump_invalid_packet(struct dp_packet *packet, const char 
*reason)
  *  of interest for the flow, otherwise UINT16_MAX.
  */
 void
-miniflow_extract(struct dp_packet *packet, struct netdev_flow_key *key)
+miniflow_extract_(struct dp_packet *packet, struct netdev_flow_key *key)
 {
 /* Add code to this function (or its callees) to extract new fields. */
 BUILD_ASSERT_DECL(FLOW_WC_SEQ == 42);
@@ -1112,6 +1114,36 @@ miniflow_extract(struct dp_packet *packet, struct 
netdev_flow_key *key)
 key->mf.map = mf.map;
 }
 
+void
+miniflow_extract(struct dp_packet *packet, struct netdev_flow_key *key)
+{
+#ifdef MFEX_AUTOVALIDATOR_DEFAULT
+static struct ovsthread_once once_enable = OVSTHREAD_ONCE_INITIALIZER;
+if (ovsthread_once_start(_enable)) {
+dpif_miniflow_extract_init();
+ovsthread_once_done(_enable);
+}
+struct dp_packet_batch packets;
+const struct pkt_metadata *md = >md;
+dp_packet_batch_init();
+dp_packet_batch_add(, packet);
+const uint32_t recirc_depth = *recirc_depth_get();
+
+/* Currently AVX512 DPIF dont support recirculation
+ * Once the support will be added the condition would
+ * be removed.
+ */
+if (recirc_depth) {
+miniflow_extract_(packet, key);
+} else {
+dpif_miniflow_extract_autovalidator(, key, 1,
+odp_to_u32(md->in_port.odp_port), NULL);
+}
+#else
+miniflow_extract_(packet, key);
+#endif
+}
+
 static ovs_be16
 parse_dl_type(const void **datap, size_t *sizep, ovs_be16 *first_vlan_tci_p)
 {
diff --git a/lib/flow.h b/lib/flow.h
index ba7c3c63a..7b277275f 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -543,6 +543,10 @@ struct pkt_metadata;
  * were extracted. */
 void
 miniflow_extract(struct dp_packet *packet, struct netdev_flow_key *key);
+
+void
+miniflow_extract_(struct dp_packet *packet, struct netdev_flow_key *key);
+
 void miniflow_map_init(struct miniflow *, const struct flow *);
 void flow_wc_map(const struct flow *, struct flowmap *);
 size_t miniflow_alloc(struct miniflow *dsts[], size_t n,
-- 
2.25.1

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

[ovs-dev] [PATCH v3 2/3] flow: Refactor miniflow_extract into api.

[Kumar Amber]

Miniflow extract used to takes the ABI parameter struct
miniflow which was removed and added inside
the struct netdev_flow_key and at many places temperory
structs were created inside the functions which could be
cleaned in favour of a uniform API.

Changing parameter to key will not affect anything as
buff array is still followed by the mf bit map inside
netdev_flow_key, thus there wont be any impact on offset
calculations which were done earlier.

Signed-off-by: Kumar Amber 

---
v3:
- Fix comments from Cian.
---
---
 lib/dpif-netdev-avx512.c  |  2 +-
 lib/dpif-netdev-private-extract.c |  2 +-
 lib/dpif-netdev.c |  2 +-
 lib/flow.c| 17 -
 lib/flow.h|  4 +++-
 ofproto/ofproto.c | 10 --
 6 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
index b7131ba3f..76eeecc9a 100644
--- a/lib/dpif-netdev-avx512.c
+++ b/lib/dpif-netdev-avx512.c
@@ -211,7 +211,7 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread 
*pmd,
 
 if (!mfex_hit) {
 /* Do a scalar miniflow extract into keys. */
-miniflow_extract(packet, >mf);
+miniflow_extract(packet, key);
 }
 
 /* Cache TCP and byte values for all packets. */
diff --git a/lib/dpif-netdev-private-extract.c 
b/lib/dpif-netdev-private-extract.c
index 4b2f12015..42b970e75 100644
--- a/lib/dpif-netdev-private-extract.c
+++ b/lib/dpif-netdev-private-extract.c
@@ -251,7 +251,7 @@ dpif_miniflow_extract_autovalidator(struct dp_packet_batch 
*packets,
 /* Run scalar miniflow_extract to get default result. */
 DP_PACKET_BATCH_FOR_EACH (i, packet, packets) {
 pkt_metadata_init(>md, in_port);
-miniflow_extract(packet, [i].mf);
+miniflow_extract(packet, [i]);
 
 /* Store known good metadata to compare with optimized metadata. */
 good_l2_5_ofs[i] = packet->l2_5_ofs;
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 139e22f38..e4e4c912b 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -8170,7 +8170,7 @@ dfc_processing(struct dp_netdev_pmd_thread *pmd,
 }
 }
 
-miniflow_extract(packet, >mf);
+miniflow_extract(packet, key);
 key->len = 0; /* Not computed yet. */
 key->hash =
 (md_is_valid == false)
diff --git a/lib/flow.c b/lib/flow.c
index dd523c889..086096d5e 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -35,6 +35,7 @@
 #include "jhash.h"
 #include "openvswitch/match.h"
 #include "dp-packet.h"
+#include "dpif-netdev-private-dpcls.h"
 #include "openflow/openflow.h"
 #include "packets.h"
 #include "odp-util.h"
@@ -633,15 +634,13 @@ parse_nsh(const void **datap, size_t *sizep, struct 
ovs_key_nsh *key)
 void
 flow_extract(struct dp_packet *packet, struct flow *flow)
 {
-struct {
-struct miniflow mf;
-uint64_t buf[FLOW_U64S];
-} m;
+
+struct netdev_flow_key key;
 
 COVERAGE_INC(flow_extract);
 
-miniflow_extract(packet, );
-miniflow_expand(, flow);
+miniflow_extract(packet, );
+miniflow_expand(, flow);
 }
 
 static inline bool
@@ -758,7 +757,7 @@ dump_invalid_packet(struct dp_packet *packet, const char 
*reason)
  *  of interest for the flow, otherwise UINT16_MAX.
  */
 void
-miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
+miniflow_extract(struct dp_packet *packet, struct netdev_flow_key *key)
 {
 /* Add code to this function (or its callees) to extract new fields. */
 BUILD_ASSERT_DECL(FLOW_WC_SEQ == 42);
@@ -767,7 +766,7 @@ miniflow_extract(struct dp_packet *packet, struct miniflow 
*dst)
 const void *data = dp_packet_data(packet);
 size_t size = dp_packet_size(packet);
 ovs_be32 packet_type = packet->packet_type;
-uint64_t *values = miniflow_values(dst);
+uint64_t *values = miniflow_values(>mf);
 struct mf_ctx mf = { FLOWMAP_EMPTY_INITIALIZER, values,
  values + FLOW_U64S };
 const char *frame;
@@ -1110,7 +1109,7 @@ miniflow_extract(struct dp_packet *packet, struct 
miniflow *dst)
 }
 }
  out:
-dst->map = mf.map;
+key->mf.map = mf.map;
 }
 
 static ovs_be16
diff --git a/lib/flow.h b/lib/flow.h
index c647ad83c..ba7c3c63a 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -41,6 +41,7 @@ struct dp_packet;
 struct ofputil_port_map;
 struct pkt_metadata;
 struct match;
+struct netdev_flow_key;
 
 /* Some flow fields are mutually exclusive or only appear within the flow
  * pipeline.  IPv6 headers are bigger than IPv4 and MPLS, and IPv6 ND packets
@@ -540,7 +541,8 @@ struct pkt_metadata;
 /* The 'dst' must follow with buffer space for FLOW_U64S 64-bit units.
  * 'dst->map' is ignored on input and set on output to indicate which fields
  * were extracted. */
-void miniflow_extract(struct dp_packet *packet, struct miniflow *dst);
+void
+miniflow_extract(struct dp_packet *packet, struct 

[ovs-dev] [PATCH v3 1/3] dpif-netdev: Refactor per thread recirc data allocation.

[Kumar Amber]

The refactor allows us to use *recirc_depth_get() to obtain
the depth across ovs which was previously limited to only
dpif-netdev.c.

Signed-off-by: Kumar Amber 
Signed-off-by: Cian Ferriter 
Co-authored-by: Cian Ferriter 
---
 lib/dpif-netdev-private-dpif.c | 2 ++
 lib/dpif-netdev-private-dpif.h | 5 +
 lib/dpif-netdev.c  | 3 ---
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/lib/dpif-netdev-private-dpif.c b/lib/dpif-netdev-private-dpif.c
index 84d4ec156..6f8de9094 100644
--- a/lib/dpif-netdev-private-dpif.c
+++ b/lib/dpif-netdev-private-dpif.c
@@ -28,6 +28,8 @@
 
 VLOG_DEFINE_THIS_MODULE(dpif_netdev_impl);
 
+DEFINE_EXTERN_PER_THREAD_DATA(recirc_depth, 0);
+
 enum dpif_netdev_impl_info_idx {
 DPIF_NETDEV_IMPL_SCALAR,
 DPIF_NETDEV_IMPL_AVX512
diff --git a/lib/dpif-netdev-private-dpif.h b/lib/dpif-netdev-private-dpif.h
index 0da639c55..15f1f36b3 100644
--- a/lib/dpif-netdev-private-dpif.h
+++ b/lib/dpif-netdev-private-dpif.h
@@ -18,6 +18,11 @@
 #define DPIF_NETDEV_PRIVATE_DPIF_H 1
 
 #include "openvswitch/types.h"
+#include "ovs-thread.h"
+
+#define MAX_RECIRC_DEPTH 6
+/* Use per thread recirc_depth to prevent recirculation loop. */
+DECLARE_EXTERN_PER_THREAD_DATA(uint32_t, recirc_depth);
 
 /* Forward declarations to avoid including files. */
 struct dp_netdev_pmd_thread;
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 676434308..139e22f38 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -97,9 +97,6 @@ VLOG_DEFINE_THIS_MODULE(dpif_netdev);
 #define MIN_TO_MSEC  6
 
 #define FLOW_DUMP_MAX_BATCH 50
-/* Use per thread recirc_depth to prevent recirculation loop. */
-#define MAX_RECIRC_DEPTH 6
-DEFINE_STATIC_PER_THREAD_DATA(uint32_t, recirc_depth, 0)
 
 /* Use instant packet send by default. */
 #define DEFAULT_TX_FLUSH_INTERVAL 0
-- 
2.25.1

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

[ovs-dev] [PATCH v3 0/3] Miniflow Extract Testing Improvements

[Kumar Amber]

The patch-set introduces changes which would improve
the testing of miniflow_extract for AVX512 based
miniflow_extract optimizations whithout affecting scalar
code path.

---
v3:
- Fix comments from Cian.
---

Kumar Amber (3):
  dpif-netdev: Refactor per thread recirc data allocation.
  flow: Refactor miniflow_extract into api.
  flow: Add autovalidator support to miniflow_extract.

 lib/dpif-netdev-avx512.c  |  2 +-
 lib/dpif-netdev-private-dpif.c|  2 ++
 lib/dpif-netdev-private-dpif.h|  5 
 lib/dpif-netdev-private-extract.c |  8 ++---
 lib/dpif-netdev.c |  5 +---
 lib/flow.c| 49 +--
 lib/flow.h|  8 -
 ofproto/ofproto.c | 10 +++
 8 files changed, 64 insertions(+), 25 deletions(-)

-- 
2.25.1

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

Re: [ovs-dev] [ovs-dev, v3, 0/8] Add support for ovs metering with tc offload

[Ilya Maximets]

On 4/27/22 04:41, Jianbo Liu wrote:
> On Wed, 2022-04-27 at 01:58 +0200, Ilya Maximets wrote:
>> On 4/26/22 09:14, Simon Horman wrote:
>>> On Tue, Apr 19, 2022 at 01:38:33AM +, Jianbo Liu via dev wrote:
 On Thu, 2022-04-07 at 09:15 +, Jianbo Liu wrote:
> This series is to add support for tc offloading of ovs
> metering, and
> enhance OVS to use new kernel feature which offload tc police
> action to
> hardware.
> To do the offloading, new APIs for meter are added in netdev-
> offload,
> and OVS meters are mapped to tc police actions with one-to-one
> relationship for dpif-netlink.
>
> Notes:
>     v3
>     - Add netdev-offload APIs for meter.
>     - Move the implementation to lower netdev-offload-tc layer.
>
>     v2
>     - Move tc police parse call from last patch to 2nd patch.
>   2nd patch is adding the parse lib func and add the call
> to it in
> that patch.
>   Last patch is the put of tc police act.
>   In 2nd patch also add empty switch case for tc police act
> put as
> the impl.
>   is done in the last patch when support for put is being
> added.
>>>
>>> ...
>>>
 Friendly ping for review.
>>>
>>> Hi Jianbo,
>>>
>>> sorry for the delayed response.
>>>
>>> My colleagues at Corigine have exercised this patchset and have
>>> verified
>>> it for our test-cases.
>>>
>>> Also, I have run the upstream test workflows over them, and they
>>> look good.
>>>
>>> 1. netdev-offload: Add meter offload provider
>>>     
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhorms2%2Fovs%2Factions%2Fruns%2F2220399423data=05%7C01%7Cjianbol%40nvidia.com%7Cbbf3220c395d4d76290e08da27e0abc6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866143161220737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7Csdata=WiPBTWZX1XsWskvmW9RPgH6VU3znLRcLq894G8cYGUw%3Dreserved=0
>>> 2. tc: Add support parsing tc police action
>>>     
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhorms2%2Fovs%2Factions%2Fruns%2F2220409233data=05%7C01%7Cjianbol%40nvidia.com%7Cbbf3220c395d4d76290e08da27e0abc6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866143161220737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7Csdata=jeMI5%2F9CN%2B2lXFEL%2Bu3L1YkENYoNB9iudLukRZHv1cU%3Dreserved=0
>>> 3. netdev-linux: Refactor put police action netlink message
>>>     
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhorms2%2Fovs%2Factions%2Fruns%2F2220516986data=05%7C01%7Cjianbol%40nvidia.com%7Cbbf3220c395d4d76290e08da27e0abc6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866143161220737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7Csdata=vXnK87IOXkdQt%2BDYG90ksy0CWZHfH7lo%2FmdHCXTu6Bc%3Dreserved=0
>>> 4. netdev-linux: Add functions to manipulate tc police action
>>>     
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhorms2%2Fovs%2Factions%2Fruns%2F2220566774data=05%7C01%7Cjianbol%40nvidia.com%7Cbbf3220c395d4d76290e08da27e0abc6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866143161220737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7Csdata=iqFPTqegSUQX35jkLzjCDavPWrzTTX3Bg%2FgZEdDCHho%3Dreserved=0
>>> 5. netdev-offload-tc: Implement and register meter offload API for
>>> tc
>>>     
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhorms2%2Fovs%2Factions%2Fruns%2F2220568491data=05%7C01%7Cjianbol%40nvidia.com%7Cbbf3220c395d4d76290e08da27e0abc6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866143161220737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7Csdata=sZ5izlE%2FYu9wfy%2B1q7wwzyn5JDE2MeH8OzTKilE7Fqg%3Dreserved=0
>>> 6. netdev-offload-tc: Cleanup police actions with reserved indexes
>>> on startup
>>>     
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhorms2%2Fovs%2Factions%2Fruns%2F2220569761data=05%7C01%7Cjianbol%40nvidia.com%7Cbbf3220c395d4d76290e08da27e0abc6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637866143161220737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7Csdata=ODy0cbf5MvUFZ%2F61%2F49wMNTzkXhOFIZinhcVQmG78I0%3Dreserved=0
>>> 7. netdev-offload-tc: Offloading rules with police actions
>>>     
>>> 

Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: remove mirror assert

[Adrian Moreno]

On 4/7/22 15:53, lic121 wrote:

During the revalidation, mirror could be removed. Instead of crash
the process, we can simply skip the deleted mirror.

Fixes: ec7ceaed4f3e ("ofproto-dpif: Modularize mirror code.")
Signed-off-by: lic121 
---
  ofproto/ofproto-dpif-xlate.c | 10 +++---
  1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 5a770f1..33e56c2 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2142,9 +2142,13 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle 
*xbundle,
  int snaplen;
  
  /* Get the details of the mirror represented by the rightmost 1-bit. */

-ovs_assert(mirror_get(xbridge->mbridge, raw_ctz(mirrors),
-  , _mirrors,
-  , , _vlan));
+if (!mirror_get(xbridge->mbridge, raw_ctz(mirrors),
+   , _mirrors,
+   , , _vlan)) {
+/* Mirror could be deleted during revalidation */
+mirrors = zero_rightmost_1bit(mirrors);
+continue;
+}
  
  
  /* If this mirror selects on the basis of VLAN, and it does not select


I have reproduced the assert and verified this patch fixes it.

The patch looks good to me. My only nit is about the comment, it's a bit 
confusing because this code is executed also in upcall handling, not only on 
revalidation. Something on the lines of the following would be a bit more clear 
IMHO:


/* The mirror got reconfigured before we got to read it's configuration. */

Also, maybe add OVS_UNLIKELY?

Thanks.
--
Adrián Moreno

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

[ovs-dev] [PATCH] faq: Fix features not available in userspace datapath.

[Cian Ferriter]

Tunnel virtual ports are supported in the userspace datapath. Update the
answer to reflect this.

Fixes: a36de779d739 ("openvswitch: Userspace tunneling.")
Signed-off-by: Cian Ferriter 
---
 Documentation/faq/releases.rst | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/faq/releases.rst b/Documentation/faq/releases.rst
index 232d19503..2c2499a27 100644
--- a/Documentation/faq/releases.rst
+++ b/Documentation/faq/releases.rst
@@ -305,9 +305,8 @@ Q: Why are UDP tunnel checksums not computed for VXLAN or 
Geneve?
 
 Q: What features are not available when using the userspace datapath?
 
-A: Tunnel virtual ports are not supported, as described in the previous
-answer.  It is also not possible to use queue-related actions.  On Linux
-kernels before 2.6.39, maximum-sized VLAN packets may not be transmitted.
+A: It is not possible to use queue-related actions.  On Linux kernels
+before 2.6.39, maximum-sized VLAN packets may not be transmitted.
 
 Q: Should userspace or kernel be upgraded first to minimize downtime?
 
-- 
2.25.1

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

Re: [ovs-dev] [PATCH v2 4/4] miniflow_extract: Add autovalidator support to miniflow_extract.

[Amber, Kumar]

Hi Cian,

Please find my replies inline.

> -Original Message-
> From: Ferriter, Cian 
> Sent: Monday, April 25, 2022 5:12 PM
> To: Amber, Kumar ; ovs-dev@openvswitch.org
> Cc: Stokes, Ian ; echau...@redhat.com;
> ktray...@redhat.com; i.maxim...@ovn.org; f...@sysclose.org; Van Haaren,
> Harry 
> Subject: RE: [PATCH v2 4/4] miniflow_extract: Add autovalidator support to
> miniflow_extract.
> 
> 
> 
> > -Original Message-
> > From: Amber, Kumar 
> > Sent: Sunday 27 March 2022 08:09
> > To: ovs-dev@openvswitch.org
> > Cc: Stokes, Ian ; echau...@redhat.com;
> > ktray...@redhat.com; i.maxim...@ovn.org; Ferriter, Cian
> > ; f...@sysclose.org; Van Haaren, Harry
> > ; Amber, Kumar 
> > Subject: [PATCH v2 4/4] miniflow_extract: Add autovalidator support to
> miniflow_extract.
> >
> > The patch adds the flag based switch between choice of using
> > miniflow_extract in normal pipeline or select mfex_autovalidator in
> > debug and test builds.
> >
> > The compile time flag used to select autoval can be done using option:
> >
> >  ./configure CFLAGS="--enable-mfex-default-autovalidator"
> >
> > Signed-off-by: Kumar Amber 
> > ---
> >  lib/dpif-netdev-private-extract.c |  4 ++--
> >  lib/flow.c| 24 
> >  2 files changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/dpif-netdev-private-extract.c
> > b/lib/dpif-netdev-private-extract.c
> > index 8538d069f..0d7091caa 100644
> > --- a/lib/dpif-netdev-private-extract.c
> > +++ b/lib/dpif-netdev-private-extract.c
> > @@ -124,8 +124,8 @@ dpif_miniflow_extract_init(void)
> >  /* For the first call, this will be choosen based on the
> >   * compile time flag.
> >   */
> > -VLOG_INFO("Default MFEX Extract implementation is %s.\n",
> > -  mfex_impls[mfex_idx].name);
> > +VLOG_DBG("Default MFEX Extract implementation is %s.\n",
> > + mfex_impls[mfex_idx].name);
> >  atomic_store_relaxed(mfex_func, (uintptr_t) mfex_impls
> >   [mfex_idx].extract_func);  } diff --git
> > a/lib/flow.c b/lib/flow.c index 127de2d7a..ddec31523 100644
> > --- a/lib/flow.c
> > +++ b/lib/flow.c
> > @@ -37,6 +37,7 @@
> >  #include "dp-packet.h"
> >  #include "dpif-netdev-private-thread.h"
> >  #include "dpif-netdev-private-dpcls.h"
> > +#include "dpif-netdev-private-extract.h"
> >  #include "openflow/openflow.h"
> >  #include "packets.h"
> >  #include "odp-util.h"
> > @@ -1121,7 +1122,30 @@ miniflow_extract_(struct dp_packet *packet,
> > struct netdev_flow_key *key)  void  miniflow_extract(struct dp_packet
> > *packet, struct netdev_flow_key *key)  {
> > +#ifdef MFEX_AUTOVALIDATOR_DEFAULT
> > +static struct ovsthread_once once_enable =
> OVSTHREAD_ONCE_INITIALIZER;
> > +if (ovsthread_once_start(_enable)) {
> > +dpif_miniflow_extract_init();
> > +ovsthread_once_done(_enable);
> > +}
> > +struct dp_packet_batch packets;
> > +const struct pkt_metadata *md = >md;
> > +dp_packet_batch_init();
> > +dp_packet_batch_add(, packet);
> > +const uint32_t recirc_depth = *recirc_depth_get();
> > +/* Currently AVX512 DPIF dont support recirculation
> > + * Once the support will be added the condition would
> > + * be removed.
> > + */
> > +if (recirc_depth) {
> 
> I'm just wondering if there is a way to avoid getting and checking the
> recirc_depth. This would avoid the need for patch 1/4 in this series. What
> about the below check instead?
> 
> if (flow_tnl_dst_is_set(>tunnel)) {
> 
> This is the check that happens inside the generic miniflow extract
> implementation. Do you think this will work instead?
> 

I tried the suggested changes but our unit tests still fails due to con-track 
thus
Recirc_depth is the best option to avoid tunnels until we support recirculation 
in DPIF.

> > +miniflow_extract_(packet, key);
> > +} else {
> > +dpif_miniflow_extract_autovalidator(, key, 1,
> > +odp_to_u32(md->in_port.odp_port),
> > + NULL);
> 
> We need to be careful about the 'NULL' being passed in here. We need to
> add checks to the use of the 'struct dp_netdev_pmd_thread *pmd_handle' in
> 'dpif_miniflow_extract_autovalidator()'.
> 

Fixed in V3.

Regards
Amber

> > +}
> > +#else
> >  miniflow_extract_(packet, key);
> > +#endif
> >  }
> >  static ovs_be16
> >  parse_dl_type(const void **datap, size_t *sizep, ovs_be16
> > *first_vlan_tci_p)
> > --
> > 2.25.1

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

[ovs-dev] [PATCH ovn] utilities: nbctl: dump lbs in load_balancer group running {ls, lr}-lb-list

[Lorenzo Bianconi]

Dump load balancers belonging to the load_balancer groups attached to the
specified logical switch/logical router running ls-lb-list/lr-lb-list.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2059261
Signed-off-by: Lorenzo Bianconi 
---
 tests/ovn-nbctl.at| 70 +--
 utilities/ovn-nbctl.c | 54 +
 2 files changed, 122 insertions(+), 2 deletions(-)

diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index f9b9defd0..2388eba2e 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -997,6 +997,30 @@ AT_CHECK([ovn-nbctl ls-lb-del ls0 lb3])
 AT_CHECK([ovn-nbctl ls-lb-list ls0 | uuidfilt], [0], [])
 AT_CHECK([ovn-nbctl --if-exists ls-lb-del ls0 lb1])
 
+AT_CHECK([ovn-nbctl lb-add lb4 40.0.0.10 162.168.10.10,162.168.10.20])
+AT_CHECK([ovn-nbctl lb-add lb5 50.0.0.10 172.168.10.10,172.168.10.20])
+AT_CHECK([ovn-nbctl lb-add lb6 60.0.0.10 182.168.10.10,182.168.10.20])
+
+lb4=$(fetch_column nb:load_balancer _uuid name=lb4)
+lb5=$(fetch_column nb:load_balancer _uuid name=lb5)
+lb6=$(fetch_column nb:load_balancer _uuid name=lb6)
+
+lbg=$(ovn-nbctl create load_balancer_group name=lbg -- \
+  add load_balancer_group lbg load_balancer $lb4 -- \
+  add load_balancer_group lbg load_balancer $lb5 -- \
+  add load_balancer_group lbg load_balancer $lb6)
+
+AT_CHECK([ovn-nbctl add logical_switch ls0 load_balancer_group $lbg])
+
+AT_CHECK([ovn-nbctl ls-lb-list ls0 | uuidfilt], [0], [dnl
+UUIDLB  PROTO  VIP 
 IPs
+<0>lb4 tcp40.0.0.10162.168.10.10,162.168.10.20
+<1>lb5 tcp50.0.0.10172.168.10.10,172.168.10.20
+<2>lb6 tcp60.0.0.10182.168.10.10,182.168.10.20
+])
+
+AT_CHECK([ovn-nbctl remove logical_switch ls0 load_balancer_group $lbg])
+
 dnl Remove all load balancers from logical switch.
 AT_CHECK([ovn-nbctl ls-lb-add ls0 lb0])
 AT_CHECK([ovn-nbctl ls-lb-add ls0 lb1])
@@ -1046,7 +1070,16 @@ AT_CHECK([ovn-nbctl ls-lb-add ls0 lb1])
 AT_CHECK([ovn-nbctl lb-del lb0])
 AT_CHECK([ovn-nbctl lb-del lb1])
 AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [])
-AT_CHECK([ovn-nbctl ls-lb-list ls0 | uuidfilt], [0], [])])
+AT_CHECK([ovn-nbctl ls-lb-list ls0 | uuidfilt], [0], [])
+
+AT_CHECK([ovn-nbctl add logical_router lr0 load_balancer_group $lbg])
+
+AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [dnl
+UUIDLB  PROTO  VIP 
 IPs
+<0>lb4 tcp40.0.0.10162.168.10.10,162.168.10.20
+<1>lb5 tcp50.0.0.10172.168.10.10,172.168.10.20
+<2>lb6 tcp60.0.0.10182.168.10.10,182.168.10.20
+])])
 
 dnl -
 
@@ -1271,6 +1304,30 @@ AT_CHECK([ovn-nbctl ls-lb-add ls0 lb3])
 AT_CHECK([ovn-nbctl ls-lb-del ls0])
 AT_CHECK([ovn-nbctl ls-lb-list ls0 | uuidfilt], [0], [])
 
+AT_CHECK([ovn-nbctl lb-add lb4 [[ae07::10]]:80 
[[fd0f::10]]:80,[[fd0f::20]]:80])
+AT_CHECK([ovn-nbctl lb-add lb5 [[ae08::10]]:80 [[fd0f::10]]:80,[[fd0f::20]]:80 
udp])
+AT_CHECK([ovn-nbctl lb-add lb6 ae09::10 fd0f::10,fd0f::20])
+
+lb4=$(fetch_column nb:load_balancer _uuid name=lb4)
+lb5=$(fetch_column nb:load_balancer _uuid name=lb5)
+lb6=$(fetch_column nb:load_balancer _uuid name=lb6)
+
+lbg=$(ovn-nbctl create load_balancer_group name=lbg -- \
+  add load_balancer_group lbg load_balancer $lb4 -- \
+  add load_balancer_group lbg load_balancer $lb5 -- \
+  add load_balancer_group lbg load_balancer $lb6)
+
+AT_CHECK([ovn-nbctl add logical_switch ls0 load_balancer_group $lbg])
+
+AT_CHECK([ovn-nbctl ls-lb-list ls0 | uuidfilt], [0], [dnl
+UUIDLB  PROTO  VIP 
 IPs
+<0>lb4 tcp[[ae07::10]]:80
[[fd0f::10]]:80,[[fd0f::20]]:80
+<1>lb5 udp[[ae08::10]]:80
[[fd0f::10]]:80,[[fd0f::20]]:80
+<2>lb6 tcpae09::10 fd0f::10,fd0f::20
+])
+
+AT_CHECK([ovn-nbctl remove logical_switch ls0 load_balancer_group $lbg])
+
 dnl Add load balancer to logical router.
 AT_CHECK([ovn-nbctl lr-add lr0])
 AT_CHECK([ovn-nbctl lr-lb-add lr0 lb0])
@@ -1305,7 +1362,16 @@ AT_CHECK([ovn-nbctl lr-lb-add lr0 lb0])
 AT_CHECK([ovn-nbctl lr-lb-add lr0 lb1])
 AT_CHECK([ovn-nbctl lr-lb-add lr0 lb3])
 AT_CHECK([ovn-nbctl lr-lb-del lr0])
-AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [])])
+AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [])
+
+AT_CHECK([ovn-nbctl add logical_router lr0 load_balancer_group $lbg])
+
+AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [dnl
+UUIDLB  PROTO  VIP 
 IPs
+<0>lb4 tcp[[ae07::10]]:80
[[fd0f::10]]:80,[[fd0f::20]]:80
+<1>lb5 udp

Re: [ovs-dev] [PATCH ovn] controller/binding: prevent claiming container lport to ovs bridge

[Mohammad Heib]

Hi Mark,
yes, you are right, this change needs to be handled in both incremental and
recompute cases i submitted v2

with recompute case which will prevent the binding from creating a
local_binding for the port in iface_id if it is a container port and will
throw a warning message.

On Tue, Apr 5, 2022 at 10:42 PM Mark Michelson  wrote:

> Hi Mohammad,
>
> The change itself seems like a good idea. I have a question. This code
> is only written for the incremental case, not the recompute case. Is
> this because this situation can only arise in the incremental case?
>
> On 3/31/22 07:01, Mohammad Heib wrote:
> > currently ovn-controller allow users to claim lport of type container
> > to ovs bridge which is invalid use-case.
> >
> > This patch will prevent such invalid use-cases by ignoring the claiming
> > requests for container lports and will throw a warning message to the
> > controller logs.
> >
> > Signed-off-by: Mohammad Heib 
> > ---
> >   controller/binding.c | 11 +++
> >   tests/ovn.at |  5 +
> >   2 files changed, 16 insertions(+)
> >
> > diff --git a/controller/binding.c b/controller/binding.c
> > index 4d62b0858..1051651f4 100644
> > --- a/controller/binding.c
> > +++ b/controller/binding.c
> > @@ -2012,6 +2012,17 @@ binding_handle_ovs_interface_changes(struct
> binding_ctx_in *b_ctx_in,
> >   int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0;
> >   if (iface_id && ofport > 0 &&
> >   is_iface_in_int_bridge(iface_rec, b_ctx_in->br_int)) {
> > +const struct sbrec_port_binding *pb = NULL;
> > +pb =
> lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name,
> > +  iface_id);
> > +if (pb && (get_lport_type(pb) == LP_CONTAINER)) {
> > +static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(1, 1);
> > +VLOG_WARN_RL(, "Can't claim lport %s of type
> container to "
> > + "OVS bridge,\nplease remove the lport
> prent_name"
> > + " before claiming it.", pb->logical_port);
> > +continue;
> > +}
> > +
> >   handled = consider_iface_claim(iface_rec, iface_id,
> b_ctx_in,
> >  b_ctx_out, qos_map_ptr);
> >   if (!handled) {
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 0c2fe9f97..d32240cf4 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -28232,6 +28232,11 @@ check ovn-nbctl --wait=sb set
> logical_switch_port vm1 parent_name=vm-cont1
> >
> >   wait_for_ports_up
> >
> > +# Try to claim container port to ovs
> > +check ovn-nbctl set logical_switch_port vm-cont2 parent_name=vm2
> > +check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=vm-cont2
> > +AT_CHECK([test 1 = `cat hv1/ovn-controller.log |grep -c "claim lport
> vm-cont2 of type container"`])
> > +
> >   # Delete vm1, vm-cont1 and vm-cont2 and recreate again.
> >   check ovn-nbctl lsp-del vm1
> >   check ovn-nbctl lsp-del vm-cont1
> >
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v2] controller/binding: prevent claiming container lport to ovs bridge

[Mohammad Heib]

currently ovn-controller allow users to claim lport of type container
to ovs bridge which is invalid use-case.

This patch will prevent such invalid use-cases by ignoring the claiming
requests for container lports and will throw a warning message to the
controller logs.

Signed-off-by: Mohammad Heib 
---
 controller/binding.c | 26 ++
 tests/ovn.at | 11 +++
 2 files changed, 37 insertions(+)

diff --git a/controller/binding.c b/controller/binding.c
index 7281b0485..19d0b3783 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -1497,6 +1497,21 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in,
 struct local_binding *lbinding =
 local_binding_find(local_bindings, iface_id);
 if (!lbinding) {
+const struct sbrec_port_binding *pb = NULL;
+pb = lport_lookup_by_name(
+b_ctx_in->sbrec_port_binding_by_name,
+iface_id);
+if (pb && (get_lport_type(pb) == LP_CONTAINER)) {
+static struct vlog_rate_limit rl =
+  VLOG_RATE_LIMIT_INIT(1, 1);
+VLOG_WARN_RL(,
+   "Can't claim lport %s of type container to "
+   "OVS bridge,\nplease remove the lport"
+   " parent_name before claiming it.",
+   pb->logical_port);
+continue;
+}
+
 lbinding = local_binding_create(iface_id, iface_rec);
 local_binding_add(local_bindings, lbinding);
 } else {
@@ -2022,6 +2037,17 @@ binding_handle_ovs_interface_changes(struct 
binding_ctx_in *b_ctx_in,
 int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0;
 if (iface_id && ofport > 0 &&
 is_iface_in_int_bridge(iface_rec, b_ctx_in->br_int)) {
+const struct sbrec_port_binding *pb = NULL;
+pb = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name,
+  iface_id);
+if (pb && (get_lport_type(pb) == LP_CONTAINER)) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
+VLOG_WARN_RL(, "Can't claim lport %s of type container to "
+ "OVS bridge,\nplease remove the lport parent_name"
+ " before claiming it.", pb->logical_port);
+continue;
+}
+
 handled = consider_iface_claim(iface_rec, iface_id, b_ctx_in,
b_ctx_out, qos_map_ptr);
 if (!handled) {
diff --git a/tests/ovn.at b/tests/ovn.at
index 34b6abfc0..bc59f3f13 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -28535,6 +28535,11 @@ check ovn-nbctl --wait=sb set logical_switch_port vm1 
parent_name=vm-cont1
 
 wait_for_ports_up
 
+# Try to claim container port to ovs
+check ovn-nbctl set logical_switch_port vm-cont2 parent_name=vm2
+check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=vm-cont2
+AT_CHECK([test 1 = `cat hv1/ovn-controller.log |grep -c "claim lport vm-cont2 
of type container"`])
+
 # Delete vm1, vm-cont1 and vm-cont2 and recreate again.
 check ovn-nbctl lsp-del vm1
 check ovn-nbctl lsp-del vm-cont1
@@ -28546,6 +28551,12 @@ check ovn-nbctl lsp-add ls vm-cont1 vm1 1
 check ovn-nbctl lsp-add ls vm-cont2 vm1 2
 
 wait_for_ports_up
+check as hv1 ovn-appctl -t ovn-controller debug/pause
+# Try to claim container port to ovs with recompute
+check ovn-nbctl set logical_switch_port vm-cont2 parent_name=vm2
+check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=vm-cont2
+check as hv1 ovn-appctl -t ovn-controller debug/resume
+AT_CHECK([test 1 = `cat hv1/ovn-controller.log |grep -c "claim lport vm-cont2 
of type container"`])
 
 # Make vm1 as a child port of some non existent lport - foo. vm1, vm1-cont1 and
 # vm1-cont2 should be released.
-- 
2.34.1

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

Re: [ovs-dev] [PATCH] Documentation: use new syntax for dpdk port representors

[0-day Robot]

Bleep bloop.  Greetings Robin Jarry, 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 92 characters long (recommended limit is 79)
#26 FILE: Documentation/topics/dpdk/phy.rst:270:
__ 
https://doc.dpdk.org/guides-21.11/prog_guide/switch_representation.html#port-representors

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


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

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

[ovs-dev] [PATCH] Documentation: use new syntax for dpdk port representors

[Robin Jarry]

Since DPDK 21.05, the representor identifier now handles a relative VF
offset. The legacy representor ID seems only valid in certain cases
(first dpdk port).

Link: https://github.com/DPDK/dpdk/commit/cebf7f17159a8
Signed-off-by: Robin Jarry 
---
 Documentation/topics/dpdk/phy.rst | 12 ++--
 lib/netdev-dpdk.c |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/topics/dpdk/phy.rst 
b/Documentation/topics/dpdk/phy.rst
index 937f4c40e5a8..8fc34a378cb8 100644
--- a/Documentation/topics/dpdk/phy.rst
+++ b/Documentation/topics/dpdk/phy.rst
@@ -267,7 +267,7 @@ Representors are multi devices created on top of one PF.
 
 For more information, refer to the `DPDK documentation`__.
 
-__ https://doc.dpdk.org/guides-21.11/prog_guide/switch_representation.html
+__ 
https://doc.dpdk.org/guides-21.11/prog_guide/switch_representation.html#port-representors
 
 Prior to port representors there was a one-to-one relationship between the PF
 and the eth device. With port representors the relationship becomes one PF to
@@ -287,18 +287,18 @@ address in devargs. For an existing bridge called ``br0`` 
and PCI address
 When configuring a VF-based port, DPDK uses an extended devargs syntax which
 has the following format::
 
-BDBF,representor=[]
+BDBF,representor=
 
 This syntax shows that a representor is an enumerated eth device (with
-a representor ID) which uses the PF PCI address.
-The following commands add representors 3 and 5 using PCI device address
+a representor identifier) which uses the PF PCI address.
+The following commands add representors of VF 3 and 5 using PCI device address
 ``:08:00.0``::
 
 $ ovs-vsctl add-port br0 dpdk-rep3 -- set Interface dpdk-rep3 type=dpdk \
-   options:dpdk-devargs=:08:00.0,representor=[3]
+   options:dpdk-devargs=:08:00.0,representor=vf3
 
 $ ovs-vsctl add-port br0 dpdk-rep5 -- set Interface dpdk-rep5 type=dpdk \
-   options:dpdk-devargs=:08:00.0,representor=[5]
+   options:dpdk-devargs=:08:00.0,representor=vf5
 
 .. important::
 
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index e0a2dccf5ce1..95f491081a11 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1772,7 +1772,7 @@ static dpdk_port_t netdev_dpdk_get_port_by_devargs(const 
char *devargs)
 }
 
 /*
- * Normally, a PCI id (optionally followed by a representor number)
+ * Normally, a PCI id (optionally followed by a representor identifier)
  * is enough for identifying a specific DPDK port.
  * However, for some NICs having multiple ports sharing the same PCI
  * id, using PCI id won't work then.
-- 
2.35.1

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

Re: [ovs-dev] [PATCH v3] ofproto-dpif-xlate: Clear out vlan flow fields while processing native tunnel.

[Thilak Raj Surendra Babu]

Hi Ilya,
No problem!
Thank you for reviewing my latest patch.
Fixed the minor review comments, changed the complex system test to a simpler 
one as per your suggestion
and submitted v4.

Thanks
Thilak Raj S

-Original Message-
From: Ilya Maximets  
Sent: 25 April 2022 14:08
To: Thilak Raj Surendra Babu ; ovs-dev@openvswitch.org
Cc: i.maxim...@ovn.org; Rosemarie O'Riorden 
Subject: Re: [PATCH v3] ofproto-dpif-xlate: Clear out vlan flow fields while 
processing native tunnel.

On 4/21/22 04:11, Thilak Raj Surendra Babu wrote:
> Hi Ilya,
> If there are no further review comments, can you please merge this patch?

Hi.

Sorry for replies taking so long, I was on PTO for a last couple
of weeks.   Unfortunately, there was no replies form others either.

See some comments inline.

> 
> Thanks
> Thilak Raj S  
> 
> 
> -Original Message-
> From: Thilak Raj Surendra Babu 
> Sent: 07 April 2022 14:08
> To: ovs-dev@openvswitch.org
> Cc: Thilak Raj Surendra Babu ; Rosemarie 
> O'Riorden 
> Subject: [PATCH v3] ofproto-dpif-xlate: Clear out vlan flow fields while 
> processing native tunnel.
> 
> When a packet is received over an access port that needs to be sent over a 
> vxlan tunnel,the access port VLAN id is used in the lookup leading to a wrong 
> packet being crafted and sent over the tunnel.
> Clear out the flow 's VLAN field as it should not be used while performing 
> mac lookup for the outer tunnel and also at this point the VLAN action 
> related to inner flow is already committed.
> 
> Version 3:
>   - Address review comments.
>   - Add unit-test patch from Rosemarie O'Riorden.
> 
> Version 2:
>   - Fixed line length warning from checkpatch.
>   - Dropped unrelated changes.

Nit:  This version log should be below the first '---', so it will not be part 
of the commit messages.

> 

Fixes: 7c12dfc527a5 ("tunneling: Avoid datapath-recirc by combining recirc 
actions at xlate.")

> Signed-off-by: Thilak Raj Surendra Babu 
> Signed-off-by: Rosemarie O'Riorden 
> Co-authored-by: Rosemarie O'Riorden 
> ---
>  ofproto/ofproto-dpif-xlate.c |  3 ++
>  tests/system-traffic.at  | 61 
>  tests/tunnel-push-pop.at | 52 ++
>  3 files changed, 116 insertions(+)
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c 
> b/ofproto/ofproto-dpif-xlate.c index 5a770f171..7ce61b176 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3542,6 +3542,9 @@ propagate_tunnel_data_to_flow__(struct flow *dst_flow,
>  dst_flow->dl_dst = dmac;
>  dst_flow->dl_src = smac;
>  
> +/* Clear VLAN entries which do not apply for tunnel flows. */
> +memset(dst_flow->vlans, 0, sizeof(dst_flow->vlans));

Nit:  Please, don't parenthesize the argument of the 'sizeof'.

> +
>  dst_flow->packet_type = htonl(PT_ETH);
>  dst_flow->nw_dst = src_flow->tunnel.ip_dst;
>  dst_flow->nw_src = src_flow->tunnel.ip_src; diff --git 
> a/tests/system-traffic.at b/tests/system-traffic.at index 
> 4a7fa49fc..f1a146b97 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -259,6 +259,67 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 
> 0.3 -w 2 10.1.1.100 | FORMAT_PI  OVS_TRAFFIC_VSWITCHD_STOP  AT_CLEANUP
>  
> +AT_SETUP([datapath - ping vlan over vxlan tunnel])
> +OVS_CHECK_TUNNEL_TSO()
> +OVS_CHECK_VXLAN()
> +
> +OVS_TRAFFIC_VSWITCHD_START()
> +ADD_BR([br-underlay])
> +
> +AT_CHECK([ovs-vsctl -- add-port br0 patch0 -- set interface patch0 
> +type=patch dnl
> +options:peer=patch1 -- add-port br-underlay patch1 -- set interface
> +patch1 dnl type=patch options:peer=patch0]) AT_CHECK([ovs-ofctl 
> +add-flow br0 "actions=normal"]) AT_CHECK([ovs-ofctl add-flow 
> +br-underlay "table=0,priority=10,actions=normal"])
> +
> +ADD_NAMESPACES(at_ns0)
> +
> +dnl Set up underlay link from host into the namespace using veth pair.
> +ADD_VETH(p0, at_ns0, br-underlay, "172.31.1.1/24") AT_CHECK([ip addr 
> +add dev br-underlay "172.31.1.100/24"]) AT_CHECK([ip link set dev 
> +br-underlay up])
> +
> +dnl Add a rule to forward arp packets towards tunnels remote endpoint 
> +AT_CHECK([ovs-ofctl add-flow br-underlay 
> +"in_port=br-underlay,priority=12,dnl
> +dl_type=0x806,nw_dst=172.31.1.1,actions=output:ovs-p0"])
> +
> +dnl Set up tunnel endpoints on OVS outside the namespace and with a 
> +native dnl linux device inside the namespace.
> +ADD_NATIVE_TUNNEL([vxlan], [at_vxlan1], [at_ns0], [172.31.1.100], 
> [10.1.1.1/24],
> +  [id 0 dstport 4789]) NS_EXEC([at_ns0], [ip link add 
> +link at_vxlan1 name at_vxlan1.100 type

NS_CHECK_EXEC() should be used instead of plain NS_EXEC().

 
> +vlan id dnl
> +100])
> +NS_EXEC([at_ns0], [ip addr flush dev at_vxlan1]) NS_EXEC([at_ns0], 
> +[ip addr add dev at_vxlan1.100 "10.1.1.30/24"]) NS_EXEC([at_ns0], [ip 
> +link set dev at_vxlan1.100 up])
> +
> +dnl add OVS tunnel
> +ADD_OVS_TUNNEL([vxlan], [br-underlay], [at_vxlan0], [172.31.1.1],
> +[10.1.1.100/24])
> +
> 

[ovs-dev] [PATCH v4] ofproto-dpif-xlate: Clear out vlan flow fields while processing native tunnel.

[Thilak Raj Surendra Babu]

When a packet is received over an access port that needs to be sent
over a vxlan tunnel,the access port VLAN id is used in the lookup
leading to a wrong packet being crafted and sent over the tunnel.
Clear out the flow 's VLAN field as it should not be used while
performing mac lookup for the outer tunnel and also at this point
the VLAN action related to inner flow is already committed.

Fixes: 7c12dfc527a5 ("tunneling: Avoid datapath-recirc by combining recirc 
actions at xlate.")
Signed-off-by: Thilak Raj Surendra Babu 
Signed-off-by: Rosemarie O'Riorden 
Co-authored-by: Rosemarie O'Riorden 

---
Version 4:
  - Address minor review comments.
  - Replace with a simpler system test.

Version 3:
  - Address review comments.
  - Add unit-test patch from Rosemarie O'Riorden.

Version 2:
  - Fixed line length warning from checkpatch.
  - Dropped unrelated changes.

 ofproto/ofproto-dpif-xlate.c |  3 +++
 tests/system-traffic.at  | 47 
 tests/tunnel-push-pop.at | 52 
 3 files changed, 102 insertions(+)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 5a770f171..fb4894935 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3542,6 +3542,9 @@ propagate_tunnel_data_to_flow__(struct flow *dst_flow,
 dst_flow->dl_dst = dmac;
 dst_flow->dl_src = smac;
 
+/* Clear VLAN entries which do not apply for tunnel flows. */
+memset(dst_flow->vlans, 0, sizeof dst_flow->vlans);
+
 dst_flow->packet_type = htonl(PT_ETH);
 dst_flow->nw_dst = src_flow->tunnel.ip_dst;
 dst_flow->nw_src = src_flow->tunnel.ip_src;
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 4a7fa49fc..d929cf985 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -259,6 +259,53 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -w 2 
10.1.1.100 | FORMAT_PI
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([datapath - ping vlan over vxlan tunnel])
+OVS_CHECK_TUNNEL_TSO()
+OVS_CHECK_VXLAN()
+
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_BR([br-underlay])
+
+AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
+AT_CHECK([ovs-ofctl add-flow br-underlay "actions=normal"])
+
+ADD_NAMESPACES(at_ns0)
+
+dnl Set up underlay link from host into the namespace using veth pair.
+ADD_VETH(p0, at_ns0, br-underlay, "172.31.2.1/24")
+AT_CHECK([ip addr add dev br-underlay "172.31.1.100/24"])
+AT_CHECK([ip link set dev br-underlay up])
+
+dnl Set up tunnel endpoints on OVS outside the namespace and with a native
+dnl linux device inside the namespace.
+ADD_OVS_TUNNEL([vxlan], [br0], [at_vxlan0], [172.31.1.1], [10.1.1.100/24])
+ADD_NATIVE_TUNNEL([vxlan], [at_vxlan1], [at_ns0], [172.31.1.100], 
[10.2.1.1/24],
+  [id 0 dstport 4789])
+
+AT_CHECK([ovs-vsctl set port br0 tag=100])
+AT_CHECK([ovs-vsctl set port br-underlay tag=42])
+
+ADD_VLAN(at_vxlan1, at_ns0, 100, "10.1.1.1/24")
+ADD_VLAN(p0, at_ns0, 42, "172.31.1.1/24")
+
+dnl First, check the underlay
+NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 172.31.1.100 | FORMAT_PING], 
[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+dnl Okay, now check the overlay with different packet sizes
+NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.100 | FORMAT_PING], 
[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+NS_CHECK_EXEC([at_ns0], [ping -s 1600 -q -c 3 -i 0.3 -w 2 10.1.1.100 | 
FORMAT_PING], [0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -w 2 10.1.1.100 | 
FORMAT_PING], [0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([datapath - ping over vxlan6 tunnel])
 OVS_CHECK_TUNNEL_TSO()
 OVS_CHECK_VXLAN_UDP6ZEROCSUM()
diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at
index 57589758f..b1bc328c3 100644
--- a/tests/tunnel-push-pop.at
+++ b/tests/tunnel-push-pop.at
@@ -842,3 +842,55 @@ Datapath actions: 7
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_SETUP([tunnel_push_pop - VXLAN access port])
+
+dnl Create bridge that has a MAC address
+OVS_VSWITCHD_START([set bridge br0 dnl
+datapath_type=dummy -- set Interface dnl
+br0 other-config:hwaddr=aa:55:aa:55:00:00])
+AT_CHECK([ovs-vsctl add-port br0 p8 -- set Interface p8 dnl
+   type=dummy ofport_request=8])
+
+dnl Create another bridge
+AT_CHECK([ovs-vsctl add-br ovs-tun0 -- set bridge ovs-tun0 
datapath_type=dummy])
+
+dnl Add VXLAN port to this bridge
+AT_CHECK([ovs-vsctl add-port ovs-tun0 tun0 -- set int tun0 dnl
+  type=vxlan options:remote_ip=10.0.0.11 -- add-port ovs-tun0 p7 dnl
+  -- set interface p7 type=dummy ofport_request=7])
+
+dnl Set VLAN tags, so that br0 and its port p8 have the same tag,
+dnl but ovs-tun0's port p7 has a different tag
+AT_CHECK([ovs-vsctl set port p8 tag=42 -- set