Re: [ovs-dev] [PATCH ovn] ovn-northd: Don't log transaction failures on standby instances.
On Thu, Jan 27, 2022 at 10:00 AM Dumitru Ceara wrote:
>
> We still need to try to ovsdb_idl_loop_commit_and_wait() on instances
> that are in standby mode. That's because they need to try to take the
> lock. But if they're in standby they won't actually build a transaction
> json and will return early because they don't own the SB lock.
>
> That's reported as an error by ovsdb_idl_loop_commit_and_wait() but we
> shouldn't act on it.
>
> Also, to ensure that no DB changes are missed, ovsdb_idl_track_clear()
> should be called only on active instances. The standby or paused ones
> will get the incremental updates when they become active. Otherwise we
> would be forced to perform a full recompute when the instance becomes
> active.
Hi Dumitru,
I've a question on the track clear being moved out of the standby instances.
To ensure correctness, I suppose it's better to trigger a full recompute when a
standby instance becomes active. What do you think?
Also lets say CMS does the below operations
- Add a logical switch S1
- Add a logical port p1 in S1
- Add a logical port p2 in S1
- Delete logical port p2
- Delete a logical switch.
With this patch since we are not clearing the tracking information,
how does ovn-northd
process the tracked changes when it becomes active ?
Thanks
Numan
>
> Fixes: 953832eb17f3 ("ovn-northd: Don't trigger recompute for transactions
> that are in progress.")
> Fixes: 4597317f16d1 ("northd: Introduce incremental processing for northd")
> Reported-by: Ilya Maximets
> Signed-off-by: Dumitru Ceara
> ---
> northd/ovn-northd.c | 35 ---
> 1 file changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 793135ede..4e1e51931 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -1089,6 +1089,26 @@ main(int argc, char *argv[])
> ovnnb_txn, ovnsb_txn,
> _idl_loop);
> }
> +
> +/* If there are any errors, we force a full recompute in
> order
> + * to ensure we handle all changes. */
> +if (!ovsdb_idl_loop_commit_and_wait(_idl_loop)) {
> +VLOG_INFO("OVNNB commit failed, "
> + "force recompute next time.");
> +recompute = true;
> +}
> +
> +if (!ovsdb_idl_loop_commit_and_wait(_idl_loop)) {
> +VLOG_INFO("OVNSB commit failed, "
> + "force recompute next time.");
> +recompute = true;
> +}
> +ovsdb_idl_track_clear(ovnnb_idl_loop.idl);
> +ovsdb_idl_track_clear(ovnsb_idl_loop.idl);
> +} else {
> +/* Make sure we send any pending requests, e.g., lock. */
> +ovsdb_idl_loop_commit_and_wait(_idl_loop);
> +ovsdb_idl_loop_commit_and_wait(_idl_loop);
> }
> } else {
> /* ovn-northd is paused
> @@ -1112,21 +1132,6 @@ main(int argc, char *argv[])
> ovsdb_idl_wait(ovnsb_idl_loop.idl);
> }
>
> -/* If there are any errors, we force a full recompute in order to
> - ensure we handle all changes. */
> -if (!ovsdb_idl_loop_commit_and_wait(_idl_loop)) {
> -VLOG_INFO("OVNNB commit failed, force recompute next time.");
> -recompute = true;
> -}
> -
> -if (!ovsdb_idl_loop_commit_and_wait(_idl_loop)) {
> -VLOG_INFO("OVNSB commit failed, force recompute next time.");
> -recompute = true;
> -}
> -
> -ovsdb_idl_track_clear(ovnnb_idl_loop.idl);
> -ovsdb_idl_track_clear(ovnsb_idl_loop.idl);
> -
> unixctl_server_run(unixctl);
> unixctl_server_wait(unixctl);
> memory_wait();
> --
> 2.27.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] ovs-sandbox: Convert initial databases if needed.
On Fri, Feb 4, 2022 at 2:34 PM Mark Michelson wrote:
>
> Acked-by: Mark Michelson
Thanks. Applied to the main branch.
Numan
>
> On 1/26/22 09:17, Dumitru Ceara wrote:
> > The --nbdb-source and --sbdb-source options allow users to point the
> > sandbox daemons to already existing database files. It's useful if
> > these are first converted to use the currently supported schema.
> >
> > Signed-off-by: Dumitru Ceara
> > ---
> > tutorial/ovs-sandbox | 12 ++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/tutorial/ovs-sandbox b/tutorial/ovs-sandbox
> > index d679cd39b..d81e00496 100755
> > --- a/tutorial/ovs-sandbox
> > +++ b/tutorial/ovs-sandbox
> > @@ -542,7 +542,11 @@ ovn_start_db() {
> > case $model in
> > standalone)
> > case $source_type in
> > -database) run cp "$source" ${db}1.db ;;
> > +database)
> > +run cp "$source" ${db}1.db
> > +run ovsdb-tool convert ${db}1.db \
> > +$srcdir/ovn-${db}.ovsschema
> > +;;
> > schema) run ovsdb-tool create ${db}1.db "$source" ;;
> > esac
> > ovn_start_ovsdb_server 1
> > @@ -551,7 +555,11 @@ ovn_start_db() {
> > backup)
> > for i in 1 2; do
> > case $source_type in
> > -database) run cp "$source" $db$i.db ;;
> > +database)
> > +run cp "$source" $db$i.db
> > +run ovsdb-tool convert $db$i.db \
> > +$srcdir/ovn-${db}.ovsschema
> > +;;
> > schema) run ovsdb-tool create $db$i.db "$source" ;;
> > esac
> > done
> >
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 ovn] controller: reconfigure ovs meters for ovn meters updates
Bleep bloop. Greetings Lorenzo Bianconi, I am a robot and I have tried out
your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
ERROR: Inappropriate bracing around statement
#100 FILE: controller/ofctrl.c:1853:
for (j = 0; j < m_installed->priv_size / sizeof *band; j++) {
WARNING: Line is 81 characters long (recommended limit is 79)
#117 FILE: controller/ofctrl.c:1870:
band = (struct sbrec_meter_band
*)m_installed->priv_data;
WARNING: Line is 81 characters long (recommended limit is 79)
#335 FILE: lib/extend-table.h:56:
void *priv_data; /* Pointer to private data (e.g. meter-bands for meters).
*/
Lines checked: 457, Warnings: 2, Errors: 1
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 v3 ovn] controller: reconfigure ovs meters for ovn meters updates
At the moment ovs meters are reconfigured by ovn just when a
new a meter is allocated while updates for an already allocated meter are
ignored. This issue can be easily verified with the following reproducer:
$ovn-nbctl meter-add meter0 drop 10 pktps
$ovn-nbctl --log --meter=meter0 acl-add sw0 to-lport 1000 'tcp.dst == 80' drop
$ovn-nbctl --may-exist meter-add meter0 drop 20 pktps
$ovs-ofctl -O OpenFlow15 dump-meters br-int
Fix the issue reconfiguring ovs meters even for ovn meters updates.
Related bz: https://bugzilla.redhat.com/show_bug.cgi?id=1939524
Signed-off-by: Lorenzo Bianconi
---
Changes since v2:
- move meter ip into lflow ip
- add new test in ovn-performance.at
- manage metering ip full-recompute
Changes since v1:
- add IP refactor to the series
- rebase on top of ovn master
---
controller/ofctrl.c | 101
controller/ofctrl.h | 3 ++
controller/ovn-controller.c | 57 +++-
lib/extend-table.c | 6 +++
lib/extend-table.h | 2 +
tests/ovn-performance.at| 15 ++
tests/ovn.at| 51 ++
tests/system-ovn.at | 17 ++
8 files changed, 230 insertions(+), 22 deletions(-)
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 08fcfed8b..c001c8ad1 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -344,8 +344,6 @@ static enum mf_field_id mff_ovn_geneve;
* is restarted, even if there is no change in the desired flow table. */
static bool need_reinstall_flows;
-static ovs_be32 queue_msg(struct ofpbuf *);
-
static struct ofpbuf *encode_flow_mod(struct ofputil_flow_mod *);
static struct ofpbuf *encode_group_mod(const struct ofputil_group_mod *);
@@ -797,7 +795,7 @@ ofctrl_get_cur_cfg(void)
return cur_cfg;
}
-static ovs_be32
+ovs_be32
queue_msg(struct ofpbuf *msg)
{
const struct ofp_header *oh = msg->data;
@@ -1802,26 +1800,12 @@ add_meter_string(struct ovn_extend_table_info
*m_desired,
}
static void
-add_meter(struct ovn_extend_table_info *m_desired,
- const struct sbrec_meter_table *meter_table,
- struct ovs_list *msgs)
+meter_create_msg(const struct sbrec_meter *sb_meter,
+ struct ovs_list *msgs, int cmd, int id)
{
-const struct sbrec_meter *sb_meter;
-SBREC_METER_TABLE_FOR_EACH (sb_meter, meter_table) {
-if (!strcmp(m_desired->name, sb_meter->name)) {
-break;
-}
-}
-
-if (!sb_meter) {
-static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-VLOG_ERR_RL(, "could not find meter named \"%s\"", m_desired->name);
-return;
-}
-
struct ofputil_meter_mod mm;
-mm.command = OFPMC13_ADD;
-mm.meter.meter_id = m_desired->table_id;
+mm.command = cmd;
+mm.meter.meter_id = id;
mm.meter.flags = OFPMF13_STATS;
if (!strcmp(sb_meter->unit, "pktps")) {
@@ -1854,6 +1838,76 @@ add_meter(struct ovn_extend_table_info *m_desired,
free(mm.meter.bands);
}
+void
+meter_update(const struct sbrec_meter *sb_meter, struct ovs_list *msgs)
+{
+struct ovn_extend_table_info *m_installed, *next_meter;
+HMAP_FOR_EACH_SAFE (m_installed, next_meter, hmap_node,
+>existing) {
+if (!strcmp(m_installed->name, sb_meter->name)) {
+struct sbrec_meter_band *band;
+
+for (int i = 0; i < sb_meter->n_bands; i++) {
+int j;
+
+for (j = 0; j < m_installed->priv_size / sizeof *band; j++) {
+band =
+(struct sbrec_meter_band *)m_installed->priv_data + j;
+if (band->rate == sb_meter->bands[i]->rate &&
+band->burst_size == sb_meter->bands[i]->burst_size) {
+break;
+}
+}
+
+if (j == m_installed->priv_size / sizeof *band) {
+meter_create_msg(sb_meter, msgs, OFPMC13_MODIFY,
+ m_installed->table_id);
+/* Recreate meter-bands cache. */
+free(m_installed->priv_data);
+m_installed->priv_size = sb_meter->n_bands * sizeof *band;
+m_installed->priv_data = xmalloc(m_installed->priv_size);
+for (i = 0; i < sb_meter->n_bands; i++) {
+band = (struct sbrec_meter_band
*)m_installed->priv_data;
+memcpy(band + i, sb_meter->bands[i], sizeof *band);
+}
+return;
+}
+}
+}
+}
+}
+
+static void
+add_meter(struct ovn_extend_table_info *m_desired,
+ const struct sbrec_meter_table *meter_table,
+ struct ovs_list *msgs)
+{
+const struct sbrec_meter *sb_meter;
+SBREC_METER_TABLE_FOR_EACH (sb_meter, meter_table) {
+if (!strcmp(m_desired->name, sb_meter->name)) {
+
Re: [ovs-dev] [PATCH ovn v3] northd: Add feature to log reply and related ACL traffic.
On Wed, Feb 2, 2022 at 8:27 PM Mark Michelson wrote:
>
> It can be desirable for replies to stateful ACLs to be logged. And in
> some cases, it can actually be a bit confusing why they aren't logged.
> Consider a situation where a port group called "port_group" exists and
> logical switch ports swp1 and swp2 belong to it. We create the following
> ACL, where logging is enabled:
>
> from-lport 100 'inport == @port_group' allow-stateless
>
> swp1 sends traffic to swp2 and swp2 responds within the same connection.
> In this case, the logs will show both the packets from swp1 to swp2, as
> well as the response packets from swp2 to swp1.
>
> Now change the ACL:
>
> from-lport 100 'inport == @port_group' allow-related
>
> Now with the same traffic pattern, the packets from swp1 to swp2 are
> logged, but the packets from swp2 to swp1 are not. Why is that?
>
> The reason is that as a shortcut, when stateful ACLs are used, a single
> priority 65532 flow is programmed to allow reply traffic to pass. When
> no stateful ACL is present, the reply traffic is at the mercy of the
> stateless ACL on the reply. Therefore, with the stateful ACL, the reply
> traffic is not actually hitting an ACL but is let through by default,
> but with the stateless ACL, the reply traffic does hit the ACL
> evaluation, so the reply traffic is logged.
>
> This change adds a feature that allows for reply traffic to be
> optionally logged for stateful ACLs, therefore allowing for the behavior
> to be similar for both ACL types. Since logging reply traffic requires
> adding more flows, it is not enabled by default. In order to have reply
> traffic logged, the ACL must have logging enabled, be stateful, have a
> label, and have the new log_related column set to true,
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2031150
>
> Signed-off-by: Mark Michelson
Hi Mark,
The patch LGTM.
Can you please update the documentation in ovn-northd.8.xml for the
new flows added.
Instead of adding a new column in the ACL table for the log direction,
how about adding options column ?
I'd suggest a generic options column.
Thanks
Numan
> ---
> v2 -> v3:
> * Seeing Dumitru's ACL direction patch, I realized check_acl_log
> shouldn't be hard-coded to think "severity" is the final ACL field.
> Tweaked script to be more resilient in the face of change.
> * Added a few more (possibly) common acl_log packet fields to check.
> * Fixed errors in system-ovn.at where I was checking the icmp_type
> instead of the icmp_code
> * Updated commit message to mention that a label is required on the ACL.
> ---
> northd/northd.c| 41 +
> ovn-nb.ovsschema | 5 +-
> ovn-nb.xml | 10 ++
> tests/automake.mk | 3 +-
> tests/check_acl_log.py | 107
> tests/ovn-northd.at| 253 +
> tests/system-ovn.at| 359 +
> 7 files changed, 775 insertions(+), 3 deletions(-)
> create mode 100644 tests/check_acl_log.py
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 22e783ff6..c1a8f71f5 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -6242,6 +6242,47 @@ consider_acl(struct hmap *lflows, struct ovn_datapath
> *od,
> acl->priority + OVN_ACL_PRI_OFFSET,
> ds_cstr(match), ds_cstr(actions),
> >header_);
> +
> +/* Related and reply traffic are universally allowed by priority
> + * 65532 flows created in build_acls(). If logging is enabled on
> + * the ACL, then we need to ensure that the related and reply
> + * traffic is logged, so we install a slightly higher-priority
> + * flow that matches the ACL, allows the traffic, and logs it.
> + */
> +if (acl->log && acl->label && acl->log_related) {
> +/* Related/reply flows need to be set on the opposite
> pipeline
> + * from where the ACL itself is set.
> + */
> +enum ovn_stage log_related_stage = ingress ?
> +S_SWITCH_OUT_ACL :
> +S_SWITCH_IN_ACL;
> +ds_clear(match);
> +ds_clear(actions);
> +
> +ds_put_format(match, "ct.est && !ct.rel && !ct.new%s && "
> + "ct.rpl && ct_label.blocked == 0 && "
> + "ct_label.label == %" PRId64,
> + use_ct_inv_match ? " && !ct.inv" : "",
> + acl->label);
> +build_acl_log(actions, acl, meter_groups);
> +ds_put_cstr(actions, "next;");
> +ovn_lflow_add_with_hint(lflows, od, log_related_stage,
> +UINT16_MAX - 2,
> +ds_cstr(match), ds_cstr(actions),
> +
Re: [ovs-dev] [PATCH net 1/1] openvswitch: Fix setting ipv6 fields causing hw csum failure
On Thu, 10 Feb 2022 10:53:24 +0200 Paul Blakey wrote: > > The calls seem a little heavy for single byte replacements. > > Can you instead add a helper based on csum_replace4() maybe? > > > > BTW doesn't pedit have the same problem? > > I don't think they are heavier then csum_replace4, csum_replace4 is a handful of instructions all of which will be inlined. csum_partial() is a function call and handles variable lengths. > but they are more bulletproof in my opinion, since they handle both > the COMPLETE and PARTIAL csum cases (in __skb_postpull_rcsum()) Yes, that's why I said "add a helper based on", a skb helper which checks the csum type of the packet but instead of calling csum_partial for no reason does the adjustment directly. > and resemble what editing of the packet should have done - pull the > header, edit, and then push it back. That's not what this code is doing, so the argument does not stand IMO. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] Set additional header in DNS message explicitly
On Tue, Feb 8, 2022 at 9:46 AM wrote: > > From: Sven Haardiek > > Some DNS Queries include an optional RR in the additional record section of > DNS, so simply copying the non-zero DNS Header for the additional records, but > not adding any leads to broken DNS packages. > > This patch explicitly sets the additional records entry in the DNS header to > 0. > > Closes #114 > Submitted-at: https://github.com/ovn-org/ovn/pull/115 > Signed-off-by: Sven Haardiek Thanks for the PR. I applied to the main branch and backported util branch-21.03 Numan > --- > controller/pinctrl.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index 293aecea2..fd0bccdb6 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -3023,8 +3023,9 @@ pinctrl_handle_dns_lookup( > /* Set the response bit to 1 in the flags. */ > out_dns_header->lo_flag |= 0x80; > > -/* Set the answer RR. */ > +/* Set the answer RRs. */ > out_dns_header->ancount = htons(ancount); > +out_dns_header->arcount = 0; > > /* Copy the Query section. */ > dp_packet_put(_out, dp_packet_data(pkt_in), dp_packet_size(pkt_in)); > -- > 2.34.1 > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ovs-dev v5] ofproto: add refcount to ofproto to fix ofproto use-after-free
On Sat, Feb 5, 2022, at 06:26, Peng He wrote:
> From hepeng:
> https://patchwork.ozlabs.org/project/openvswitch/patch/20200717015041.82746-1-hepeng.0...@bytedance.com/#2487473
>
> also from guohongzhi :
> http://patchwork.ozlabs.org/project/openvswitch/patch/20200306130555.19884-1-guohongz...@huawei.com/
>
> also from a discussion about the mixing use of RCU and refcount in the mail
> list with Ilya Maximets, William Tu, Ben Pfaf, and Gaëtan Rivet.
>
> A summary, as quoted from Ilya:
>
> "
> RCU for ofproto was introduced for one
> and only one reason - to avoid freeing ofproto while rules are still
> alive. This was done in commit f416c8d61601 ("ofproto: RCU postpone
> rule destruction."). The goal was to allow using rules without
> refcounting them within a single grace period. And that forced us
> to postpone destruction of the ofproto for a single grace period.
> Later commit 39c9459355b6 ("Use classifier versioning.") made it
> possible for rules to be alive for more than one grace period, so
> the commit made ofproto wait for 2 grace periods by double postponing.
> As we can see now, that wasn't enough and we have to wait for more
> than 2 grace periods in certain cases.
> "
>
> In a short, the ofproto should have a longer life time than rule, if
> the rule lasts for more than 2 grace periods, the ofproto should live
> longer to ensure rule->ofproto is valid. It's hard to predict how long
> a ofproto should live, thus we need to use refcount on ofproto to make
> things easy. The controversial part is that we have already used RCU postpone
> to delay ofproto destrution, if we have to add refcount, is it simpler to
> use just refcount without RCU postpone?
>
> IMO, I think going back to the pure refcount solution is more
> complicated than mixing using both.
>
> Gaëtan Rive asks some questions on guohongzhi's v2 patch:
>
> during ofproto_rule_create, should we use ofproto_ref
> or ofproto_try_ref? how can we make sure the ofproto is alive?
>
> By using RCU, ofproto has three states:
>
> state 1: alive, with refcount >= 1
> state 2: dying, with refcount == 0, however pointer is valid
> state 3: died, memory freed, pointer might be dangling.
>
> Without using RCU, there is no state 2, thus, we have to be very careful
> every time we see a ofproto pointer. In contrast, with RCU, we can be sure
> that it's alive at least in this grace peroid, so we can just check if
> it is dying by ofproto_try_ref.
>
> This shows that by mixing use of RCU and refcount we can save a lot of work
> worrying if ofproto is dangling.
>
> In short, the RCU part makes sure the ofproto is alive when we use it,
> and the refcount part makes sure it lives longer enough.
>
> In this patch, I have merged guohongzhi's patch and mine, and fixes
> accoring to the previous comments.
>
> v4->v5:
> * fix the comments, remove the ref to wangyunjian's patch and
> remove the comments describing the previous ofproto destruction code.
> * fix group alloc leak issues.
>
> Signed-off-by: Peng He
> Signed-off-by: guohongzhi
> Acked-by: Mike Pattrick
> ---
> ofproto/ofproto-dpif-xlate-cache.c | 2 ++
> ofproto/ofproto-dpif-xlate.c | 14
> ofproto/ofproto-dpif.c | 24 +++--
> ofproto/ofproto-provider.h | 2 ++
> ofproto/ofproto.c | 57 +++---
> ofproto/ofproto.h | 4 +++
> 6 files changed, 82 insertions(+), 21 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate-cache.c
> b/ofproto/ofproto-dpif-xlate-cache.c
> index dcc91cb38..9224ee2e6 100644
> --- a/ofproto/ofproto-dpif-xlate-cache.c
> +++ b/ofproto/ofproto-dpif-xlate-cache.c
> @@ -209,6 +209,7 @@ xlate_cache_clear_entry(struct xc_entry *entry)
> {
> switch (entry->type) {
> case XC_TABLE:
> +ofproto_unref(&(entry->table.ofproto->up));
> break;
> case XC_RULE:
> ofproto_rule_unref(>rule->up);
> @@ -231,6 +232,7 @@ xlate_cache_clear_entry(struct xc_entry *entry)
> free(entry->learn.ofm);
> break;
> case XC_NORMAL:
> +ofproto_unref(&(entry->normal.ofproto->up));
> break;
> case XC_FIN_TIMEOUT:
> /* 'u.fin.rule' is always already held as a XC_RULE, which
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 6fb59e170..129cdf714 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3024,12 +3024,14 @@ xlate_normal(struct xlate_ctx *ctx)
> struct xc_entry *entry;
>
> /* Save just enough info to update mac learning table later. */
> -entry = xlate_cache_add_entry(ctx->xin->xcache, XC_NORMAL);
> -entry->normal.ofproto = ctx->xbridge->ofproto;
> -entry->normal.in_port = flow->in_port.ofp_port;
> -entry->normal.dl_src = flow->dl_src;
> -entry->normal.vlan = vlan;
> -entry->normal.is_gratuitous_arp = is_grat_arp;
> +if (ofproto_try_ref(>xbridge->ofproto->up)) {
> +
Re: [ovs-dev] [ovs-dev v2] tunnel: Remove padding from packet when encapsulating.
On 2/10/22 02:21, Tonghao Zhang wrote:
> On Tue, Jan 25, 2022 at 3:16 PM Harold Huang wrote:
>>
>> Looks good to me.
>>
>> Reviewed-by: Harold Huang
> Hi Ilya, do you have an opinion?
>
>> 于2022年1月25日周二 13:25写道：
>>>
>>> From: Tonghao Zhang
>>>
>>> The old version of openvswitch doesn't remove the padding from
>>> packet before L3+ conntrack processing and then packets are dropped
>>> in linux kernel stack. The patch [1] fixes the issue. We fix this
>>> issue on gateway which running ovs-dpdk as a quick workaround. Padding
>>> should be removed because tunnel size + inner size > 64B.
Hi. So, the sequence of events is following:
1. Packet arrives to the OVS userspace datapath, packet has padding.
2. OVS encapsulates it and sends to other host that runs OVS with kernel
datapath. The kernel version is old and doesn't have fix [1], i.e
kernel version is not 4.9.104+, 4.14.100+ or 4.16+.
3. Kernel datapath decapsulates the packet and sends to conntrack.
4. conntrack drops the packet as invalid, because it doesn't expect
L2 padding.
Is that correct?
Some comments inline.
>>> More detailes, see [1]
>>>
>>> [1] -
>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=9382fe71c0058465e942a633869629929102843d
>>> Signed-off-by: Tonghao Zhang
>>> ---
>>> v2: add OVS_UNLIKELY
>>> v1: this version was submitted a year ago
>>> http://patchwork.ozlabs.org/project/openvswitch/patch/20201214030936.87354-1-xiangxia.m@gmail.com/
>>> ---
>>> lib/netdev-native-tnl.c | 4
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
>>> index b89dfdd52a86..e23d71d4aec1 100644
>>> --- a/lib/netdev-native-tnl.c
>>> +++ b/lib/netdev-native-tnl.c
>>> @@ -149,11 +149,15 @@ void *
>>> netdev_tnl_push_ip_header(struct dp_packet *packet,
>>> const void *header, int size, int *ip_tot_size)
>>> {
>>> +int padding = dp_packet_l2_pad_size(packet);
>>> struct eth_header *eth;
>>> struct ip_header *ip;
>>> struct ovs_16aligned_ip6_hdr *ip6;
>>>
>>> eth = dp_packet_push_uninit(packet, size);
>>> +if (OVS_UNLIKELY(padding)) {
Would be great to have a short comment here saying why we need to trim the
packet.
e.g. "Older kernels may drop the packet after decapsulation if it contains
unexpected L2 padding."
>>> +dp_packet_set_size(packet, dp_packet_size(packet) - padding);
It doesn't seem intuitive to trim after we pushed the header. Can we do
that before instead?
>>> +}
>>> *ip_tot_size = dp_packet_size(packet) - sizeof (struct eth_header);
>>>
>>> memcpy(eth, header, size);
>>> --
>>> 2.27.0
>>>
>>> ___
>>> dev mailing list
>>> d...@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] Intel AVX-512 CI
This is great! Congrats to all of you that work to get this up and running. fbl On Tue, Feb 8, 2022 at 5:01 PM Aaron Conole wrote: > Hi Michael, > > "Phelan, Michael" writes: > > > Hi all, > > > > As presented at the OVS conference in December, the AVX-512 CI is coming > online today to test incoming > > patches to the OVS mailing list with AVX-512 implementations enabled. > The CI will download and apply > > patches before compiling OVS with DPCLS, DPIF and MFEX AVX-512 > implementations and then running the > > OVS DPDK unit tests to verify functionality. We are currently not > running the MFEX Autovalidator Fuzzy unit > > test as there seems to be some issues with warnings that causes the test > to fail unexpectedly. The results of > > the test for a patch will be sent to the ovs-build mailing list and if > there is failure in the test the author of the > > patch will be Cc’d also. > > > > > > > > Please feel free to get in touch with me with any feedback you may have > or if you notice any unexpected > > results. > > I see some patches have been tested already. Thanks for this work, it's > great to see other labs integrating for public CI. Kudos to all the > work! > > > Kind regards, > > > > Michael Phelan. > > > > > > > > -- > > > > Intel Research and Development Ireland Limited Registered in Ireland > > > > Registered Office: Collinstown Industrial Park, Leixlip, County > > > > Kildare Registered Number: 308263 > > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] system-dpdk.at: Add warning log in mfex fuzzy test.
> -Original Message- > From: dev On Behalf Of Kumar Amber > Sent: Wednesday 9 February 2022 09:50 > To: ovs-dev@openvswitch.org > Cc: Amber, Kumar ; david.march...@redhat.com > Subject: [ovs-dev] [PATCH v3] system-dpdk.at: Add warning log in mfex fuzzy > test. > > Some specific warning are seen on various systems > which may not be visible on others but good to add > such logs to test to avoid test-case failure. > > Thw warning only effects the fuzzy tests due to > more than 1000+ flows being offloading simultanously. > > Wilcarding flow count number as for different systems > under test the number could vary in the warning log. > > Suggested-by: Eelco Chaudron > Signed-off-by: Kumar Amber > I didn't see the issue that this patch fixes initially. However, if I make the below change, I can see the error during the test. Change: diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at index 9384cf7f0..165b68f0e 100644 --- a/tests/system-dpdk.at +++ b/tests/system-dpdk.at @@ -256,6 +256,8 @@ AT_KEYWORDS([dpdk]) AT_SKIP_IF([! $PYTHON3 -c "import scapy"], [], []) AT_CHECK([$PYTHON3 $srcdir/mfex_fuzzy.py $srcdir], [], [stdout]) OVS_DPDK_START() +AT_CHECK([ovs-vsctl --no-wait set Open_vSwitch . other_config:flow-limit=13500]) + dnl Add userspace bridge and attach it to OVS AT_CHECK([ovs-vsctl add-br br0 -- set bridge br0 datapath_type=netdev]) Error: > 2022-02-10T09:42:11.350Z|6|ofproto_dpif_upcall(pmd-c21/id:101)|WARN|upcall: > datapath reached the dynamic limit of 13500 flows. Which causes a fail: 7. system-dpdk.at:254: FAILED (system-dpdk.at:280) Applying this patch while keeping the above diff to generate the flow limit error results in the test passing. I can see the "reached the dynamic limit" warning in the ovs-vswitchd.log but it doesn't cause a test failure. Finding the value of "13500" as a flow-limit took some experimentation and would probably vary based on platform. With that, and looking at the .at changes, LGTM. Acked-by: Cian Ferriter ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net 1/1] openvswitch: Fix setting ipv6 fields causing hw csum failure
On Wed, 9 Feb 2022, Jakub Kicinski wrote:
> On Mon, 7 Feb 2022 16:41:01 +0200 Paul Blakey wrote:
> > Ipv6 ttl, label and tos fields are modified without first
> > pulling/pushing the ipv6 header, which would have updated
> > the hw csum (if available). This might cause csum validation
> > when sending the packet to the stack, as can be seen in
> > the trace below.
> >
> > Fix this by calling postpush/postpull checksum calculation
> > which will update the hw csum if needed.
>
> > -static void set_ipv6_fl(struct ipv6hdr *nh, u32 fl, u32 mask)
> > +static void set_ipv6_dsfield(struct sk_buff *skb, struct ipv6hdr *nh, __u8
> > ipv6_tclass, __u8 mask)
> > {
> > + skb_postpull_rcsum(skb, nh, 4);
> > +
> > + ipv6_change_dsfield(nh, ~mask, ipv6_tclass);
> > +
> > + skb_postpush_rcsum(skb, nh, 4);
> > +}
>
> The calls seem a little heavy for single byte replacements.
> Can you instead add a helper based on csum_replace4() maybe?
>
> BTW doesn't pedit have the same problem?
>
I don't think they are heavier then csum_replace4, but they are more
bulletproof in my opinion, since they handle both the COMPLETE and PARTIAL
csum cases (in __skb_postpull_rcsum()) and resemble what editing of the
packet should have done - pull the header, edit, and then push it back.
RE pedit, not really the issue here, but i guess pedit should be followed
by act_csum with the relevant checksum fields (as we do when offloading
ovs set() action to tc pedit + csum actions).
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
