Bleep bloop. Greetings Ben Pfaff, 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: Comment with 'xxx' marker
#3723 FILE: northd/ovn-northd-ddlog.c:166:
.cs =
Bleep bloop. Greetings Ben Pfaff, 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: Unexpected sign-offs from developers who are not authors or co-authors
or committers:
The idea is to run each test twice, once with ovn-northd, once
with ovn-northd-ddlog. To do that, we add a macro OVN_FOR_EACH_NORTHD
and bracket each test that uses ovn-northd in it.
Signed-off-by: Ben Pfaff
Acked-by: Dumitru Ceara
---
tests/ovn-ic.at | 11 +-
tests/ovn-macros.at | 96
Also available here:
https://github.com/blp/ovs-reviews/tree/ddlog19
v10:
- Fix linking problem reported by Dumitru and others.
- Update to ddlog 0.36.0 (thanks Leonid!).
- Port OVN commits up to 858d1dd716db ("ofctrl: Fix the assert seen when
flood removing flows.")
- Fix IGMP and MLD
I don't know a good reason to intentionally create an empty multicast
group, but disallowing empty multicast groups has an odd side effect:
you can delete all but one of the ports that a group contains and the
database will happily remove the ports from the group automatically
(because 'ports' is
Otherwise this test is racy because it assumes that northd finishes
its updates between the ovn-nbctl changes and the subsequent checks.
Also, simplify some series of "check_column" into single
"check_row_count" calls (that then become "wait_row_count").
Signed-off-by: Ben Pfaff
---
From: Leonid Ryzhyk
Export `ddlog_warn` and `ddlog_err` functions that are just wrappers
around `VLOG_WARN` and `VLOG_ERR`. This is not ideal because the
functions are exported by `ovn_util.c` and the resulting log messages use
`ovn_util` as module name. More importantly, these functions do
The code here checked whether 'bfd_e' was null even though that wasn't
necessary given the control flow just above it.
Signed-off-by: Ben Pfaff
---
northd/ovn-northd.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
On Wed, Feb 17, 2021 at 2:36 PM Tim Rozet wrote:
>
>
>
>
> On Tue, Feb 16, 2021 at 4:21 AM Han Zhou wrote:
>>
>>
>>
>> On Wed, Feb 10, 2021 at 2:17 PM Lorenzo Bianconi <
lorenzo.bianc...@redhat.com> wrote:
>> >
>> > Increase priority for automatic routes (routes created assigning IP
>> >
> On Tue, Feb 16, 2021 at 4:21 AM Han Zhou wrote:
>>
>>
>>
>> On Wed, Feb 10, 2021 at 2:17 PM Lorenzo Bianconi
>> wrote:
>> >
>> > Increase priority for automatic routes (routes created assigning IP
>> > addresses to OVN logical router interfaces) in order to always prefer them
>> > over static
On Tue, Feb 16, 2021 at 4:21 AM Han Zhou wrote:
>
>
> On Wed, Feb 10, 2021 at 2:17 PM Lorenzo Bianconi <
> lorenzo.bianc...@redhat.com> wrote:
> >
> > Increase priority for automatic routes (routes created assigning IP
> > addresses to OVN logical router interfaces) in order to always prefer
>
On 1/27/21 12:03 PM, Ilya Maximets wrote:
> On 1/27/21 11:42 AM, Dumitru Ceara wrote:
>> On 1/22/21 8:04 PM, Ilya Maximets wrote:
>>> This commit is based on a similar one from OVN by Dumitru Ceara:
>>> a429b24f7bf5 ("ci: Enable AddressSanitizer in Linux clang CI test runs.")
>>>
>>> It's
Thanks William and YiHung for review, I sent a new version.
On Tue, Feb 16, 2021 at 8:45 PM William Tu wrote:
> On Tue, Feb 16, 2021 at 1:40 PM Yi-Hung Wei wrote:
> >
> > On Tue, Feb 16, 2021 at 1:06 PM Yifeng Sun
> wrote:
> > >
> > > ovs-vswitchd could crash under these circumstances:
> > >
ovs-vswitchd could crash under these circumstances:
1. When one bridge is being destroyed, ofproto_destroy() is called and
connmgr pointer of its ofproto struct is nullified. This ofproto struct is
deallocated through 'ovsrcu_postpone(ofproto_destroy_defer__, p);'.
2. Before RCU enters quiesce
In order to help understanding system behaviour for debugging purpose,
introduce coverage counters for run handlers of ovn-controller
I-P engine nodes. Moreover add a global counter for engine abort.
https://bugzilla.redhat.com/show_bug.cgi?id=1890902
Signed-off-by: Lorenzo Bianconi
---
Changes
On Wed, Feb 17, 2021 at 8:40 AM Ilya Maximets wrote:
>
> On 1/18/21 5:12 PM, Ilya Maximets wrote:
> > Since commit 79eadafeb1b4 udpif_stop_threads() doesn't delete datapath
> > flows while called from udpif_destroy(). This means that ukeys are
> > not cleaned up either. So, hash maps in
On 17/02/2021 15:17, Balazs Nemeth wrote:
> Signed-off-by: Balazs Nemeth
> ---
> Documentation/tutorials/ipsec.rst | 21 -
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/tutorials/ipsec.rst
> b/Documentation/tutorials/ipsec.rst
> index
>
> >> Information Rate). High priority traffic is routed to queue 10,
> >> which marks
> >> all traffic as CIR, i.e. Green. All low priority traffic, queue 20,
> >> is
> >> marked as EIR, i.e. Yellow::
> >>
> >> $ ovs-vsctl --timeout=5 set port dpdk1 qos=@myqos -- \
> >>
On 17 Feb 2021, at 18:17, William Tu wrote:
On Wed, Feb 17, 2021 at 8:21 AM Eelco Chaudron
wrote:
On 17 Feb 2021, at 9:23, Eelco Chaudron wrote:
On 17 Feb 2021, at 4:39, William Tu wrote:
RFC4115 says "The CIR and EIR are both measured in bits/s."
Fix the example use case based on
On Wed, Feb 17, 2021 at 8:21 AM Eelco Chaudron wrote:
>
>
>
> On 17 Feb 2021, at 9:23, Eelco Chaudron wrote:
>
> > On 17 Feb 2021, at 4:39, William Tu wrote:
> >
> >> RFC4115 says "The CIR and EIR are both measured in bits/s."
> >> Fix the example use case based on the decription.
> >> 64-Byte
Do not add noise to the DBG log for empty sweeps.
Only log time taken when some connections were cleaned.
Signed-off-by: Gaetan Rivet
Reviewed-by: Eli Britstein
---
lib/conntrack.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/lib/conntrack.c b/lib/conntrack.c
index
The lock priority order is for the global 'ct_lock' to be taken first
and then 'conn->lock'. This is an issue, as multiple operations on
connections are thus blocked between threads contending on the
global 'ct_lock'.
This was previously necessary due to how the expiration lists, timeout
policies
When ct_sweep() is far behind on its work, the 'next_wake' returned can
be before the moment it started. When it happens, the thread schedules a
zero ms timer that is logged as an error.
Instead, mark the thread for immediate wake in the next poll_block().
Signed-off-by: Gaetan Rivet
The current rate limit is set to allow other threads to update the
connections when applicable. This was valid when taking the 'ct_lock'
was needed with a global critical section.
Now that the size of the critical section for 'ct_lock' is reduced, it
is not necessary to rate limit calls to
Change the data structure from hmap to cmap for zone limits.
As they are shared amongst multiple conntrack users, multiple
readers want to check the current zone limit state before progressing in
their processing. Using a CMAP allows doing lookups without taking the
global 'ct_lock', thus reducing
Multiple lookups are done to stored timeout policies, each time blocking
the global 'ct_lock'. This is usually not necessary and it should be
acceptable to get policy updates slightly delayed (by one RCU sync
at most). Using a CMAP reduces multiple lock taking and releasing in
the connection
Change the connection expiration lists from ovs_list to rculist.
This is a pre-step towards reducing the granularity of 'ct_lock'.
Signed-off-by: Gaetan Rivet
Reviewed-by: Eli Britstein
---
lib/conntrack-private.h | 76 +++--
lib/conntrack-tp.c | 60
A lock is taken during conn_lookup() to check whether a connection is
expired before returning it. This lock can have some contention.
Even though this lock ensures a consistent sequence of writes, it does
not imply a specific order. A ct_clean thread taking the lock first
could read a value that
Conntracks are executed within the datapath. Locks along this path are crucial
and their critical section should be minimal. The global 'ct_lock' is necessary
before any action taken on connection states. This lock is needed for many
operations on the conntrack, slowing down the datapath.
The
The 'hash_basis' field is used sometimes during sub-systems init
routine. It will be 0 by default before randomization. Sub-systems would
then init some nodes with incorrect hash values.
The timeout policies module is affected, making the default policy being
referenced using an incorrect hash
On 2/17/21 2:49 AM, Ben Pfaff wrote:
On Fri, Dec 11, 2020 at 03:43:22PM +0100, Dumitru Ceara wrote:
diff --git a/tests/ovn.at b/tests/ovn.at
index 6e396895826f..2e4eaf7f12f7 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -16982,6 +16982,10 @@ AT_CLEANUP
OVN_FOR_EACH_NORTHD([
On 1/18/21 5:12 PM, Ilya Maximets wrote:
> Since commit 79eadafeb1b4 udpif_stop_threads() doesn't delete datapath
> flows while called from udpif_destroy(). This means that ukeys are
> not cleaned up either. So, hash maps in udpif->ukeys[] might still
> contain valid pointers to ukeys that
On 2/16/21 10:16 PM, Ben Pfaff wrote:
On Fri, Dec 11, 2020 at 03:43:22PM +0100, Dumitru Ceara wrote:
There are a few tests that fail (also northd-C tests) and I suspect it's
because the OVS IDL split patches [0] this commit depends on don't
include some of the recent fixes that went into the
On 2/17/21 5:24 PM, Mark Gray wrote:
On 16/02/2021 15:53, Dumitru Ceara wrote:
Depending on the logical flow matches, multiple SB flows can point to
the same desired flow. If it happens that the desired flow conflicts
with a more restrictive (already installed) flow, then we shouldn't try
to
On 16/02/2021 15:53, Dumitru Ceara wrote:
> Depending on the logical flow matches, multiple SB flows can point to
> the same desired flow. If it happens that the desired flow conflicts
> with a more restrictive (already installed) flow, then we shouldn't try
> to add the desired flow multiple
On 17 Feb 2021, at 9:23, Eelco Chaudron wrote:
On 17 Feb 2021, at 4:39, William Tu wrote:
RFC4115 says "The CIR and EIR are both measured in bits/s."
Fix the example use case based on the decription.
64-Byte packet * 8 * 1000 pps = 512000
Did you run some tests to verify the changes you
On 2/17/21 2:47 AM, William Tu wrote:
> On Tue, Feb 16, 2021 at 2:27 PM Ilya Maximets wrote:
>>
>> While decoding RAW_ENCAP action, decode_ed_prop() might re-allocate
>> ofpbuf if there is no enough space left. However, function
>> 'decode_NXAST_RAW_ENCAP' continues to use old pointer to 'encap'
William Tu writes:
> Hi Aaron,
>
> Should we also consider the case where udp checksum is 0x?
> I saw in netdev_tnl_calc_udp_csum, we set to 0x when a packet has
> udp checksum = 0.
I can add it. This was added because a PMD we caught had a broken
firmware that didn't support udp with
Signed-off-by: Balazs Nemeth
---
Documentation/tutorials/ipsec.rst | 21 -
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/Documentation/tutorials/ipsec.rst
b/Documentation/tutorials/ipsec.rst
index 3b3e42c59..b6cc1c3a8 100644
---
Signed-off-by: Balazs Nemeth
---
Documentation/tutorials/ipsec.rst | 29 -
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/Documentation/tutorials/ipsec.rst
b/Documentation/tutorials/ipsec.rst
index 3b3e42c59..6bcf3842e 100644
---
Thanks Numan, some suggestions below!
On 09/02/2021 18:44, num...@ovn.org wrote:
> From: Numan Siddique
>
> When a Gateway router is configured with a load balancer
> and it is also configured with options:lb_force_snat_ip=,
> OVN after load balancing the destination IP to one of the
> backend
On 17/02/2021 10:40, 贺鹏 wrote:
> Hi,
>
> Thanks for the review.
>
> Mark Gray 于2021年2月17日周三 下午6:13写道：
>>
>> I'm not too familiar with this code but I have some comments.
>>
>> On 15/02/2021 09:50, Peng He wrote:
>>> CT zone could be set from a field that is not included in frozen
>>> metedata.
Hi,
Thanks for the review.
Mark Gray 于2021年2月17日周三 下午6:13写道：
>
> I'm not too familiar with this code but I have some comments.
>
> On 15/02/2021 09:50, Peng He wrote:
> > CT zone could be set from a field that is not included in frozen
> > metedata. Consider the belowing cases which is normally
I'm not too familiar with this code but I have some comments.
On 15/02/2021 09:50, Peng He wrote:
> CT zone could be set from a field that is not included in frozen
> metedata. Consider the belowing cases which is normally used in
Nits:
s/metedata/metadata
s/belowing cases which is/cases below
On 17 Feb 2021, at 4:39, William Tu wrote:
RFC4115 says "The CIR and EIR are both measured in bits/s."
Fix the example use case based on the decription.
64-Byte packet * 8 * 1000 pps = 512000
Did you run some tests to verify the changes you made?
Fixes: e61bdffc2a98 ("netdev-dpdk: Add new
45 matches
Mail list logo
