[ovs-dev] Director Finance & Investment
Good Day , I am Dr Saleem Ahmed Saleef from A UAE Based Investment Group. We are expanding our global presence by investing in projects/investments outside the United Arab Emirates in the form of debt finance only/Soft Loan. We grant our funding at a guaranteed 3% interest per Annum for projects 2 years term above to 15years, We have the financial capacity to fund projects/investments between $500,000 -$4.5 Billion USD as loan, so if you have any viable project,business and interested in us funding it as loan, i'll be happy to have your response in my email so i can forward the funding application form and procedure for your review and follow up. Waiting for your mail for further details and discussion so as to proceed. Shukran and Allah Bless. Dr Saleem Ahmed Saleef Director Finance & Investment Al Nakheel P.O.Box 5662,Ras Al Khaimah,United Arab Emirates ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] 答复: ipf question
Thanks, Darrell I can try to test it Other question, it seems that ipf is driven by input packet, no timer, if a packet has 40 fragment, first time, 32 fragments are received, next time, 8 fragment are received, reassemble is success, and send out 32 fragment. After that, if no new packets are received, the remaining 8 fragment will in complete list, and can not send out until expired to clean them, is it true? -RongQing 发件人: Darrell Ball [mailto:dlu...@gmail.com] 发送时间: 2019年11月19日 10:22 收件人: Li,Rongqing 抄送: ovs dev 主题: Re: ipf question Thanks; I had a look and I noticed ipf does not keep all the context it needs to properly resume fragment processing in the general case; I have a potential fix, but won't get to it this week. On Sun, Nov 17, 2019 at 11:08 PM Darrell Ball mailto:dlu...@gmail.com>> wrote: On Fri, Nov 15, 2019 at 6:03 PM Li,Rongqing mailto:lirongq...@baidu.com>> wrote: 发件人: Darrell Ball mailto:dlu...@gmail.com>> 发送时间: 2019年11月15日 22:58 收件人: Li,Rongqing 抄送: ovs dev 主题: Re: ipf question >Let me paraphrase, just to confirm we are on the same page. >IIUC, for example, in the case of a 33 fragment packet, in the first pass all >33 fragments enter ipf, then are >reassembled, pass thru conntrack >and then the frags sent out, while in the second pass, only 32 fragments enter >conntrack/ipf, while index 32 >fragment is being forwarded out without going >thru conntrack/ipf ? true. the second pass is recirculation and index 32 fragment is not into contrack/ipf, and send out to vm directly. can you check what rule is being hit by that fragment packet vs others and then compare the pkt metadata if I change NETDEV_MAX_BURST to 64, it works good test thanks -RongQing ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] Comment financer sa formation : découvrez le Guide 2020 des dispositifs de financement
Logo Office Academy FORMATION PROFESSIONNELLE Guide 2020 des dispositifs de financement Salaris, demandeurs d’emploi, chefs d’entreprise, agents de l’Etat… Quel que soit votre statut, il existe en France de nombreuses solutions pour financer vos projets de formation. TÉLÉCHARGER LE GUIDE Une question ? Contactez nous au 01 86 95 27 81 SSi vous ne souhaitez plus recevoir de messages envoys par ASCPM, cliquez-ici Vous disposez dun droit daccs, de rectification, dopposition et de consentement auquel vous avez accs partir de cette page web : Charte des Donnes Personnelles. Vous recevez ce message car vous tes prsent dans la base ASCPM compose de dirigeants et professionnels. ASCPM - 5 Avenue du Gnral de Gaulle - 94160 Saint Mand - France - R.C.S. 814 073 060 - CRETEIL ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] ipf question
Thanks; I had a look and I noticed ipf does not keep all the context it needs to properly resume fragment processing in the general case; I have a potential fix, but won't get to it this week. On Sun, Nov 17, 2019 at 11:08 PM Darrell Ball wrote: > > > On Fri, Nov 15, 2019 at 6:03 PM Li,Rongqing wrote: > >> 发件人: Darrell Ball >> 发送时间: 2019年11月15日 22:58 >> 收件人: Li,Rongqing >> 抄送: ovs dev >> 主题: Re: ipf question >> >> >> >Let me paraphrase, just to confirm we are on the same page. >> >IIUC, for example, in the case of a 33 fragment packet, in the first >> pass all 33 fragments enter ipf, then are >reassembled, pass thru conntrack >> >and then the frags sent out, while in the second pass, only 32 fragments >> enter conntrack/ipf, while index 32 >fragment is being forwarded out >> without going thru conntrack/ipf ? >> >> true. >> the second pass is recirculation >> and index 32 fragment is not into contrack/ipf, and send out to vm >> directly. >> > > can you check what rule is being hit by that fragment packet vs others and > then compare the pkt metadata > > >> if I change NETDEV_MAX_BURST to 64, it works >> > > good test > > >> >> thanks >> >> -RongQing >> >> >> >> >> > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 2/2] cmap: non-blocking cmap lookup
Hi Ilya,
New node insertion does not always set bucket slot. It should be only two cases
would trigger slot updating.
1. node is inserted into an empty slot.
2. rearrange cmap bucket in case of no candidate bucket for new node. see
"cmap_insert_bfs"
1st case is an empty slot, there is no other node in the list.
2nd case, the slot movement has two step:
a. copy an slot(hash,node list) into another candidate slot.
b. replace the old position with another slot.
So there is at least one complete slot copy in the cmap. If one slot is
updating and skipped by bitmap, it will be found in anther candidate bucket.
Best Regards,
Wei Yanqin
> -Original Message-
> From: Ilya Maximets
> Sent: Tuesday, November 19, 2019 12:06 AM
> To: Ola Liljedahl ; i.maxim...@ovn.org; Yanqin Wei
> (Arm Technology China) ; ovs-dev@openvswitch.org
> Cc: Gavin Hu (Arm Technology China) ; nd
> ; b...@ovn.org
> Subject: Re: [ovs-dev] [PATCH v1 2/2] cmap: non-blocking cmap lookup
>
> On 18.11.2019 16:55, Ola Liljedahl wrote:
> > On Mon, 2019-11-18 at 16:45 +0100, Ilya Maximets wrote:
> >> On 18.11.2019 3:45, Yanqin Wei wrote:
> >>
> >> Currently cmap_bucket is protected by a counter. Cmap reader will be
> >> blocked until the counter becomes even, writer increments it by 1 to
> >> be odd, and after slot update, increments by 1 again to become even.
> >> If the writer is pending or scheduled out during the writer course,
> >> the reader will be blocked.
> >>
> >> This patch introduces a bitmap as guard variable for (hash,node) pair.
> >> Writer need set slot valid bit to 0 before each change. And after
> >> change, writer sets valid bit to 1 and increments counter. Thus,
> >> reader can ensure slot consistency by only scanning valid slot and
> >> then checking counter+bitmap does not change while examining the bucket.
> >>
> >> The only time a reader has to retry the read operation for single
> >> bucket is when 1)a writer clears a bit in the valid bitmap between a
> >> reader's first and second read of the counter+bitmap.
> >> 2)a writer increments the counter between a reader's first and second
> >> read of counter+bitmap.
> >> I.e. the read operation needs to be retried when some other thread
> >> has made progress (with a write).
> >> There is no spinning/waiting for other threads to complete. This
> >> makes the design non-blocking (for readers).
> >>
> >> And it has almost no additional overhead because counter and bitmap
> >> share one 32 bits variable. No additional load/store for reader and
> >> writer.
> >>
> >>
> >> IIUC, after this change if writer will start inserting the node,
> >> reader will not find any node with the same hash in cmap because it
> >> will check only "valid" slots. This breaks the cmap API and could lead to
> crash.
> > Why wouldn't readers find other valid slots with the same hash value?
> > It is only the slot that is being updated that is cleared in the valid
> > bitmap duing the update. Other valid slots (irrespective of actual
> > hash values) are unchanged.
> >
> > Am I missing something? Do the users of cmap have some other
> > expectations that are not obvious from looking at the cmap code?
>
> bucket->nodes[i] is not a single node, but the list of nodes with the
> bucket->same hash
> equal to bucket->hashes[i].
>
> You may look at the implementation of
> CMAP_NODE_FOR_EACH/CMAP_FOR_EACH_WITH_HASH
> and read comments to 'struct cmap_node' and cmap.{c,h} in general.
>
> While adding the new node to the list you're restricting access to the whole
> list
> making it impossible to find any node there.
>
> Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] northd: Match IPv4 or IPv6 for MAC resolution
While debugging some problems in a cluster using ovn-kubernetes, I
noticed that we're creating two conflicting logical flows. These two
flows only matched on the destination MAC address. It was not
deterministic whether you'd hit the IPv4 (ARP) or IPv6 (NS) version.
This change adds an ip4 or ip6 match to each flow as appropriate.
Signed-off-by: Russell Bryant
---
northd/ovn-northd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--- NOTE ---
I've only tested this by running "make check" and "make check-kernel" so
far, and all tests still pass.
If I'm reading this code right, I'm really surprised this hasn't come up
sooner? I guess we also don't have adequate test coverage for these
flows?
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 41e97f841..f0ab43b27 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -9319,7 +9319,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap
*ports,
}
ovn_lflow_add(lflows, od, S_ROUTER_IN_ARP_REQUEST, 100,
- "eth.dst == 00:00:00:00:00:00",
+ "eth.dst == 00:00:00:00:00:00 && ip4",
"arp { "
"eth.dst = ff:ff:ff:ff:ff:ff; "
"arp.spa = reg1; "
@@ -9328,7 +9328,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap
*ports,
"output; "
"};");
ovn_lflow_add(lflows, od, S_ROUTER_IN_ARP_REQUEST, 100,
- "eth.dst == 00:00:00:00:00:00",
+ "eth.dst == 00:00:00:00:00:00 && ip6",
"nd_ns { "
"nd.target = xxreg0; "
"output; "
--
2.23.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5 ovn 2/4] ovn-controller: Add per node states to I-P engine.
Thanks Dumitru. Please see my comments inline below.
On Mon, Nov 18, 2019 at 6:07 AM Dumitru Ceara wrote:
>
> This commit transforms the 'changed' field in struct engine_node in a
> 'state' field. Possible node states are:
> - "Stale": data in the node is not up to date with the DB.
> - "Updated": data in the node is valid but was updated during
> the last run of the engine.
> - "Valid": data in the node is valid and didn't change during
> the last run of the engine.
> - "Aborted": during the last run, processing was aborted for
> this node.
>
> This commit also further refactors the I-P engine:
> - instead of recursively performing all the engine processing a
> preprocessing stage is added (engine_get_nodes()) before the main
processing
> loop is executed in order to topologically sort nodes in the engine such
> that all inputs of a given node appear in the sorted array before the
node
> itself. This simplifies a bit the code in engine_run().
Could you tell the reason of changing it to non-recursive? It seems adding
more code rather than simplifying, and effort is needed to ensure the
correctness for the new code. Probably there are some benefit that make the
later patches easier, but it is not obvious to me. Could you help point out
if that's the case?
> - remove the need for using an engine_run_id by using the newly added
states.
>
> Signed-off-by: Dumitru Ceara
> ---
> controller/ovn-controller.c | 88 ++---
> lib/inc-proc-eng.c | 219
---
> lib/inc-proc-eng.h | 75 +++
> 3 files changed, 271 insertions(+), 111 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index c56190f..033eff4 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -758,10 +758,10 @@ en_ofctrl_is_connected_run(struct engine_node *node)
> (struct ed_type_ofctrl_is_connected *)node->data;
> if (data->connected != ofctrl_is_connected()) {
> data->connected = !data->connected;
> -node->changed = true;
> +engine_set_node_state(node, EN_UPDATED);
> return;
> }
> -node->changed = false;
> +engine_set_node_state(node, EN_VALID);
> }
>
> struct ed_type_addr_sets {
> @@ -811,7 +811,7 @@ en_addr_sets_run(struct engine_node *node)
> addr_sets_init(as_table, >addr_sets);
>
> as->change_tracked = false;
> -node->changed = true;
> +engine_set_node_state(node, EN_UPDATED);
> }
>
> static bool
> @@ -830,11 +830,14 @@ addr_sets_sb_address_set_handler(struct engine_node
*node)
> addr_sets_update(as_table, >addr_sets, >new,
> >deleted, >updated);
>
> -node->changed = !sset_is_empty(>new) ||
!sset_is_empty(>deleted)
> -|| !sset_is_empty(>updated);
> +if (!sset_is_empty(>new) || !sset_is_empty(>deleted) ||
> +!sset_is_empty(>updated)) {
> +engine_set_node_state(node, EN_UPDATED);
> +} else {
> +engine_set_node_state(node, EN_VALID);
> +}
>
> as->change_tracked = true;
> -node->changed = true;
> return true;
> }
>
> @@ -885,7 +888,7 @@ en_port_groups_run(struct engine_node *node)
> port_groups_init(pg_table, >port_groups);
>
> pg->change_tracked = false;
> -node->changed = true;
> +engine_set_node_state(node, EN_UPDATED);
> }
>
> static bool
> @@ -904,11 +907,14 @@ port_groups_sb_port_group_handler(struct
engine_node *node)
> port_groups_update(pg_table, >port_groups, >new,
> >deleted, >updated);
>
> -node->changed = !sset_is_empty(>new) ||
!sset_is_empty(>deleted)
> -|| !sset_is_empty(>updated);
> +if (!sset_is_empty(>new) || !sset_is_empty(>deleted) ||
> +!sset_is_empty(>updated)) {
> +engine_set_node_state(node, EN_UPDATED);
> +} else {
> +engine_set_node_state(node, EN_VALID);
> +}
>
> pg->change_tracked = true;
> -node->changed = true;
> return true;
> }
>
> @@ -1091,7 +1097,7 @@ en_runtime_data_run(struct engine_node *node)
> update_ct_zones(local_lports, local_datapaths, ct_zones,
> ct_zone_bitmap, pending_ct_zones);
>
> -node->changed = true;
> +engine_set_node_state(node, EN_UPDATED);
> }
>
> static bool
> @@ -1157,10 +1163,10 @@ en_mff_ovn_geneve_run(struct engine_node *node)
> enum mf_field_id mff_ovn_geneve = ofctrl_get_mf_field_id();
> if (data->mff_ovn_geneve != mff_ovn_geneve) {
> data->mff_ovn_geneve = mff_ovn_geneve;
> -node->changed = true;
> +engine_set_node_state(node, EN_UPDATED);
> return;
> }
> -node->changed = false;
> +engine_set_node_state(node, EN_VALID);
> }
>
> struct ed_type_flow_output {
> @@ -1322,7 +1328,7 @@ en_flow_output_run(struct engine_node *node)
> active_tunnels,
> flow_table);
>
> -node->changed =
Re: [ovs-dev] [PATCH v5 ovn 0/4] Refactor I-P engine and fix use after free.
On Mon, Nov 18, 2019 at 1:27 PM Mark Michelson wrote:
>
> Hi Dumitru,
>
> I reviewed this changeset as a whole rather than trying to review each
> individual patch.
>
> Why does runtime_data_sb_port_binding_handler() in ovn-controller.c not
> update node state? Other change handlers update the node state based on
> whether data has changed. Is there some special reason why this
> particular change handler does not or cannot update node state? I'm
> guessing this is done on purpose, but because there is no comment it
> seems like an omission.
Hi Mark,
I can answer this. This function never changes the data, simply because it
didn't implement it, but if it is sure that the change has no impact, it
will return true, meaning the change has been handled properly (and there
is no impact to this node). Maybe I should have added some comment in the
beginning since this is not very obvious.
With the original implementation, the node->changed is always initialized
to false, so not setting it means keep the default "false".
With the new implementation, the node->state is always set to EN_VALID at
the end of engine_run_node().
Thanks,
Han
>
> On 11/18/19 9:06 AM, Dumitru Ceara wrote:
> > The incremental processing engine might stop a run before the
> > en_runtime_data node is processed. In such cases the ed_runtime_data
> > fields might contain pointers to already deleted SB records. For
> > example, if a port binding corresponding to a patch port is removed from
> > the SB database and the incremental processing engine aborts before the
> > en_runtime_data node is processed then the corresponding local_datapath
> > hashtable entry in ed_runtime_data is stale and will store a pointer to
> > the already freed sbrec_port_binding record.
> >
> > This will cause invalid memory accesses in various places (e.g.,
> > pinctrl_run() -> prepare_ipv6_ras()).
> >
> > This series fixes the issue (patch4) but to make the fix generic and
> > easier to debug it first refactors the incremental processing engine in
> > the following way:
> > - patch1: split engine_run() in smaller functional parts and simplify
the
> >logic of calling engine_run and engine_need_run in the main loop.
> > - patch2: remove recursion from the I-P engine code. Introduce node
states
> >to track validity of node data.
> > - patch3: move ct-zones to its own engine node in order to remove
dependencies
> >on other runtime data.
> >
> > CC: Han Zhou
> > Fixes: ca278d98a4f5 ("ovn-controller: Initial use of incremental engine
- quiet mode.")
> > Signed-off-by: Dumitru Ceara
> >
> > Dumitru Ceara (4):
> >ovn-controller: Refactor I-P engine_run() tracking.
> >ovn-controller: Add per node states to I-P engine.
> >ovn-controller: Add separate I-P engine node for processing
ct-zones.
> >ovn-controller: Fix use of dangling pointers in I-P runtime_data.
> >
> >
> > controller/ovn-controller.c | 418
---
> > lib/inc-proc-eng.c | 314 +---
> > lib/inc-proc-eng.h | 104 +--
> > 3 files changed, 564 insertions(+), 272 deletions(-)
> >
> > ---
> > v5:
> > - Rebase.
> > v4:
> > - Address Numan's comments:
> > - Fix engine_need_run().
> > v3:
> > - split the change in series.
> > - Address Han's comments:
> > - fix the data encapsulation issue.
> > - add is_valid method to nodes.
> > - add internal_data/data fields to nodes as it makes it easier to
write
> >the code instead of adding an "engine_get_data()" API.
> > v2: Address Han's comments:
> > - call engine_node_valid() in all the places where node local data is
> >used.
> > - move out "global" data outside the engine nodes. Make a clear
> >separation between data that can be safely used at any time and data
> >that can be used only when the engine run was successful.
> > - add a debug log for iterations when the engine didn't run.
> > - refactor a bit more the incremental engine code.
> >
> > ___
> > 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 net 2/2] act_ct: support asymmetric conntrack
On Mon, Nov 18, 2019 at 04:21:39PM -0500, Aaron Conole wrote:
> Marcelo Ricardo Leitner writes:
>
> > On Fri, Nov 08, 2019 at 04:07:14PM -0500, Aaron Conole wrote:
> >> The act_ct TC module shares a common conntrack and NAT infrastructure
> >> exposed via netfilter. It's possible that a packet needs both SNAT and
> >> DNAT manipulation, due to e.g. tuple collision. Netfilter can support
> >> this because it runs through the NAT table twice - once on ingress and
> >> again after egress. The act_ct action doesn't have such capability.
> >>
> >> Like netfilter hook infrastructure, we should run through NAT twice to
> >> keep the symmetry.
> >>
> >> Fixes: b57dc7c13ea9 ("net/sched: Introduce action ct")
> >>
> >> Signed-off-by: Aaron Conole
> >> ---
> >> net/sched/act_ct.c | 13 -
> >> 1 file changed, 12 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> >> index fcc46025e790..f3232a00970f 100644
> >> --- a/net/sched/act_ct.c
> >> +++ b/net/sched/act_ct.c
> >> @@ -329,6 +329,7 @@ static int tcf_ct_act_nat(struct sk_buff *skb,
> >> bool commit)
> >> {
> >> #if IS_ENABLED(CONFIG_NF_NAT)
> >> + int err;
> >>enum nf_nat_manip_type maniptype;
> >>
> >>if (!(ct_action & TCA_CT_ACT_NAT))
> >> @@ -359,7 +360,17 @@ static int tcf_ct_act_nat(struct sk_buff *skb,
> >>return NF_ACCEPT;
> >>}
> >>
> >> - return ct_nat_execute(skb, ct, ctinfo, range, maniptype);
> >> + err = ct_nat_execute(skb, ct, ctinfo, range, maniptype);
> >> + if (err == NF_ACCEPT &&
> >> + ct->status & IPS_SRC_NAT && ct->status & IPS_DST_NAT) {
> >> + if (maniptype == NF_NAT_MANIP_SRC)
> >> + maniptype = NF_NAT_MANIP_DST;
> >> + else
> >> + maniptype = NF_NAT_MANIP_SRC;
> >> +
> >> + err = ct_nat_execute(skb, ct, ctinfo, range, maniptype);
> >> + }
> >
> > I keep thinking about this and I'm not entirely convinced that this
> > shouldn't be simpler. More like:
> >
> > if (DNAT)
> > DNAT
> > if (SNAT)
> > SNAT
> >
> > So it always does DNAT before SNAT, similarly to what iptables would
> > do on PRE/POSTROUTING chains.
>
> I can rewrite the whole function, but I wanted to start with the smaller
> fix that worked. I also think it needs more testing then (since it's
> something of a rewrite of the function).
>
> I guess it's not too important - do you think it gives any readability
> to do it this way? If so, I can respin the patch changing it like you
> describe.
I didn't mean a rewrite, but just to never handle SNAT before DNAT. So
the fix here would be like:
- return ct_nat_execute(skb, ct, ctinfo, range, maniptype);
+ err = ct_nat_execute(skb, ct, ctinfo, range, maniptype);
+ if (err == NF_ACCEPT && maniptype == NF_NAT_MANIP_DST &&
+ ct->status & IPS_SRC_NAT && ct->status & IPS_DST_NAT) {
+ maniptype = NF_NAT_MANIP_SRC;
+ err = ct_nat_execute(skb, ct, ctinfo, range, maniptype);
+ }
+ return err;
> >> + return err;
> >> #else
> >>return NF_ACCEPT;
> >> #endif
> >> --
> >> 2.21.0
> >>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v3] vagrant: Use centos-8 box from official location
Apologies, I did not see that you corrected this in v4. On 11/18/19 4:55 PM, Mark Michelson wrote: Hi Flavio, What's the difference between v2 and v3? 0-day robot is complaining because your git-configured e-mail address is different from the one in the Signed-off-by: line. On 11/17/19 8:11 PM, Flavio Fernandes wrote: From: Flavio Fernandes Prefer the usage of "vagrant box update" instead of "dnf update" to save provisioning time. Also, as part of provisioning centos-8, use pip to install packages twisted, zope-interface and six. Signed-off-by: Flavio Fernandes --- Vagrantfile | 11 +-- 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/Vagrantfile b/Vagrantfile index 88c981c71..f0526ec48 100644 --- a/Vagrantfile +++ b/Vagrantfile @@ -37,7 +37,7 @@ apt-get -y install build-essential fakeroot graphviz autoconf automake bzip2 \ SCRIPT $bootstrap_ovs_centos7 =
Re: [ovs-dev] [PATCH ovn v3] vagrant: Use centos-8 box from official location
Hi Flavio, What's the difference between v2 and v3? 0-day robot is complaining because your git-configured e-mail address is different from the one in the Signed-off-by: line. On 11/17/19 8:11 PM, Flavio Fernandes wrote: From: Flavio Fernandes Prefer the usage of "vagrant box update" instead of "dnf update" to save provisioning time. Also, as part of provisioning centos-8, use pip to install packages twisted, zope-interface and six. Signed-off-by: Flavio Fernandes --- Vagrantfile | 11 +-- 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/Vagrantfile b/Vagrantfile index 88c981c71..f0526ec48 100644 --- a/Vagrantfile +++ b/Vagrantfile @@ -37,7 +37,7 @@ apt-get -y install build-essential fakeroot graphviz autoconf automake bzip2 \ SCRIPT $bootstrap_ovs_centos7 =
Re: [ovs-dev] [PATCH v5 ovn 0/4] Refactor I-P engine and fix use after free.
Hi Dumitru,
I reviewed this changeset as a whole rather than trying to review each
individual patch.
Why does runtime_data_sb_port_binding_handler() in ovn-controller.c not
update node state? Other change handlers update the node state based on
whether data has changed. Is there some special reason why this
particular change handler does not or cannot update node state? I'm
guessing this is done on purpose, but because there is no comment it
seems like an omission.
On 11/18/19 9:06 AM, Dumitru Ceara wrote:
The incremental processing engine might stop a run before the
en_runtime_data node is processed. In such cases the ed_runtime_data
fields might contain pointers to already deleted SB records. For
example, if a port binding corresponding to a patch port is removed from
the SB database and the incremental processing engine aborts before the
en_runtime_data node is processed then the corresponding local_datapath
hashtable entry in ed_runtime_data is stale and will store a pointer to
the already freed sbrec_port_binding record.
This will cause invalid memory accesses in various places (e.g.,
pinctrl_run() -> prepare_ipv6_ras()).
This series fixes the issue (patch4) but to make the fix generic and
easier to debug it first refactors the incremental processing engine in
the following way:
- patch1: split engine_run() in smaller functional parts and simplify the
logic of calling engine_run and engine_need_run in the main loop.
- patch2: remove recursion from the I-P engine code. Introduce node states
to track validity of node data.
- patch3: move ct-zones to its own engine node in order to remove dependencies
on other runtime data.
CC: Han Zhou
Fixes: ca278d98a4f5 ("ovn-controller: Initial use of incremental engine - quiet
mode.")
Signed-off-by: Dumitru Ceara
Dumitru Ceara (4):
ovn-controller: Refactor I-P engine_run() tracking.
ovn-controller: Add per node states to I-P engine.
ovn-controller: Add separate I-P engine node for processing ct-zones.
ovn-controller: Fix use of dangling pointers in I-P runtime_data.
controller/ovn-controller.c | 418 ---
lib/inc-proc-eng.c | 314 +---
lib/inc-proc-eng.h | 104 +--
3 files changed, 564 insertions(+), 272 deletions(-)
---
v5:
- Rebase.
v4:
- Address Numan's comments:
- Fix engine_need_run().
v3:
- split the change in series.
- Address Han's comments:
- fix the data encapsulation issue.
- add is_valid method to nodes.
- add internal_data/data fields to nodes as it makes it easier to write
the code instead of adding an "engine_get_data()" API.
v2: Address Han's comments:
- call engine_node_valid() in all the places where node local data is
used.
- move out "global" data outside the engine nodes. Make a clear
separation between data that can be safely used at any time and data
that can be used only when the engine run was successful.
- add a debug log for iterations when the engine didn't run.
- refactor a bit more the incremental engine code.
___
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 net 2/2] act_ct: support asymmetric conntrack
Paul Blakey writes:
> On 11/14/2019 4:22 PM, Roi Dayan wrote:
>>
>> On 2019-11-08 11:07 PM, Aaron Conole wrote:
>>> The act_ct TC module shares a common conntrack and NAT infrastructure
>>> exposed via netfilter. It's possible that a packet needs both SNAT and
>>> DNAT manipulation, due to e.g. tuple collision. Netfilter can support
>>> this because it runs through the NAT table twice - once on ingress and
>>> again after egress. The act_ct action doesn't have such capability.
>>>
>>> Like netfilter hook infrastructure, we should run through NAT twice to
>>> keep the symmetry.
>>>
>>> Fixes: b57dc7c13ea9 ("net/sched: Introduce action ct")
>>>
>>> Signed-off-by: Aaron Conole
>>> ---
>>> net/sched/act_ct.c | 13 -
>>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
>>> index fcc46025e790..f3232a00970f 100644
>>> --- a/net/sched/act_ct.c
>>> +++ b/net/sched/act_ct.c
>>> @@ -329,6 +329,7 @@ static int tcf_ct_act_nat(struct sk_buff *skb,
>>> bool commit)
>>> {
>>> #if IS_ENABLED(CONFIG_NF_NAT)
>>> + int err;
>>> enum nf_nat_manip_type maniptype;
>>>
>>> if (!(ct_action & TCA_CT_ACT_NAT))
>>> @@ -359,7 +360,17 @@ static int tcf_ct_act_nat(struct sk_buff *skb,
>>> return NF_ACCEPT;
>>> }
>>>
>>> - return ct_nat_execute(skb, ct, ctinfo, range, maniptype);
>>> + err = ct_nat_execute(skb, ct, ctinfo, range, maniptype);
>>> + if (err == NF_ACCEPT &&
>>> + ct->status & IPS_SRC_NAT && ct->status & IPS_DST_NAT) {
>>> + if (maniptype == NF_NAT_MANIP_SRC)
>>> + maniptype = NF_NAT_MANIP_DST;
>>> + else
>>> + maniptype = NF_NAT_MANIP_SRC;
>>> +
>>> + err = ct_nat_execute(skb, ct, ctinfo, range, maniptype);
>>> + }
>>> + return err;
>>> #else
>>> return NF_ACCEPT;
>>> #endif
>>>
>> +paul
>
> Hi Aaron,
>
> I think I understand the issue and this looks good,
>
> Can you describe the scenario to reproduce this?
It reproduces with OpenShift 3.10, which makes forward direction packets
between namespaces pump through a tun device that applies NAT rules to
rewrite the dest. Limit the namespace number of ephemeral sockets using
by editing net.ipv4.ip_local_port_range in the client namespace, and
connect to the server namespace. That's the mechanism for OvS. But for
TC I guess there wouldn't be anything convenient avaiable.
I'll try to script up something that doesn't use openshift.
>
> Thanks,
>
> Paul.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] rhel: Fix ovs-kmod-manage.sh that may create invalid soft links
On Mon, Nov 18, 2019 at 12:32:30PM -0800, William Tu wrote:
> On Mon, Nov 18, 2019 at 12:19:26PM -0800, Yi-Hung Wei wrote:
> > On Mon, Nov 18, 2019 at 12:06 PM Yifeng Sun wrote:
> > >
> > > Current code iterates every kernel under '/lib/modules' for a matched
> > > version. As a result, this script may create invalid soft links if the
> > > matched kernel doesn't have openvswitch-kmod RPM installed.
> > >
> > > This patch fixes it.
> > >
> > > VMWare-BZ: #2257534
> > >
> > > Fixes: c3570519 ("rhel: add 4.4 kernel in kmod build with mulitple
> > > versions, fedora")
> > > Signed-off-by: Yifeng Sun
> >
> > LGTM,
> >
> > Acked-by: Yi-Hung Wei
>
> Thanks, I applied to master.
> William
Also backport to branch-2.12
William
> > ___
> > 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 net 2/2] act_ct: support asymmetric conntrack
Marcelo Ricardo Leitner writes:
> On Fri, Nov 08, 2019 at 04:07:14PM -0500, Aaron Conole wrote:
>> The act_ct TC module shares a common conntrack and NAT infrastructure
>> exposed via netfilter. It's possible that a packet needs both SNAT and
>> DNAT manipulation, due to e.g. tuple collision. Netfilter can support
>> this because it runs through the NAT table twice - once on ingress and
>> again after egress. The act_ct action doesn't have such capability.
>>
>> Like netfilter hook infrastructure, we should run through NAT twice to
>> keep the symmetry.
>>
>> Fixes: b57dc7c13ea9 ("net/sched: Introduce action ct")
>>
>> Signed-off-by: Aaron Conole
>> ---
>> net/sched/act_ct.c | 13 -
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
>> index fcc46025e790..f3232a00970f 100644
>> --- a/net/sched/act_ct.c
>> +++ b/net/sched/act_ct.c
>> @@ -329,6 +329,7 @@ static int tcf_ct_act_nat(struct sk_buff *skb,
>>bool commit)
>> {
>> #if IS_ENABLED(CONFIG_NF_NAT)
>> +int err;
>> enum nf_nat_manip_type maniptype;
>>
>> if (!(ct_action & TCA_CT_ACT_NAT))
>> @@ -359,7 +360,17 @@ static int tcf_ct_act_nat(struct sk_buff *skb,
>> return NF_ACCEPT;
>> }
>>
>> -return ct_nat_execute(skb, ct, ctinfo, range, maniptype);
>> +err = ct_nat_execute(skb, ct, ctinfo, range, maniptype);
>> +if (err == NF_ACCEPT &&
>> +ct->status & IPS_SRC_NAT && ct->status & IPS_DST_NAT) {
>> +if (maniptype == NF_NAT_MANIP_SRC)
>> +maniptype = NF_NAT_MANIP_DST;
>> +else
>> +maniptype = NF_NAT_MANIP_SRC;
>> +
>> +err = ct_nat_execute(skb, ct, ctinfo, range, maniptype);
>> +}
>
> I keep thinking about this and I'm not entirely convinced that this
> shouldn't be simpler. More like:
>
> if (DNAT)
> DNAT
> if (SNAT)
> SNAT
>
> So it always does DNAT before SNAT, similarly to what iptables would
> do on PRE/POSTROUTING chains.
I can rewrite the whole function, but I wanted to start with the smaller
fix that worked. I also think it needs more testing then (since it's
something of a rewrite of the function).
I guess it's not too important - do you think it gives any readability
to do it this way? If so, I can respin the patch changing it like you
describe.
>> +return err;
>> #else
>> return NF_ACCEPT;
>> #endif
>> --
>> 2.21.0
>>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net 1/2] openvswitch: support asymmetric conntrack
Nicolas Dichtel writes:
> Le 08/11/2019 à 22:07, Aaron Conole a écrit :
>> The openvswitch module shares a common conntrack and NAT infrastructure
>> exposed via netfilter. It's possible that a packet needs both SNAT and
>> DNAT manipulation, due to e.g. tuple collision. Netfilter can support
>> this because it runs through the NAT table twice - once on ingress and
>> again after egress. The openvswitch module doesn't have such capability.
>>
>> Like netfilter hook infrastructure, we should run through NAT twice to
>> keep the symmetry.
>>
>> Fixes: 05752523e565 ("openvswitch: Interface with NAT.")
>> Signed-off-by: Aaron Conole
> In this case, ovs_ct_find_existing() won't be able to find the
> conntrack, right?
vswitchd normally won't allow both actions to get programmed. Even the
kernel module won't allow it, so this really will only happen when the
connection gets established via the nf_hook path, and then needs to be
processed via openvswitch. In those cases, the tuple lookup should be
correct, because the nf_nat table should contain the correct tuple data,
and the skbuff should have the correct tuples in the packet data to
begin with.
> Inverting the tuple to find the conntrack doesn't work anymore with double
> NAT.
> Am I wrong?
I think since the packet was double-NAT on the way out (via nf_hook
path), then the incoming reply will have the correct NAT tuples and the
lookup will happen just fine. Just that during processing, both
transformations aren't applied.
Makes sense?
> Regards,
> Nicolas
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 2/2] cmap: non-blocking cmap lookup
On Mon, 2019-11-18 at 20:18 +0100, Ilya Maximets wrote:
> On 18.11.2019 17:24, Ola Liljedahl wrote:
>
> On Mon, 2019-11-18 at 17:06 +0100, Ilya Maximets wrote:
>
> On 18.11.2019 16:55, Ola Liljedahl wrote:
>
> On Mon, 2019-11-18 at 16:45 +0100, Ilya Maximets wrote:
>
> On 18.11.2019 3:45, Yanqin Wei wrote:
>
> Currently cmap_bucket is protected by a counter. Cmap reader will be
> blocked until the counter becomes even, writer increments it by 1 to be
> odd, and after slot update, increments by 1 again to become even. If the
> writer is pending or scheduled out during the writer course, the reader
> will be blocked.
>
> This patch introduces a bitmap as guard variable for (hash,node) pair.
> Writer need set slot valid bit to 0 before each change. And after change,
> writer sets valid bit to 1 and increments counter. Thus, reader can ensure
> slot consistency by only scanning valid slot and then checking counter+bitmap
> does not change while examining the bucket.
>
> The only time a reader has to retry the read operation for single bucket
> is when
> 1)a writer clears a bit in the valid bitmap between a reader's first and
> second read of the counter+bitmap.
> 2)a writer increments the counter between a reader's first and second
> read of counter+bitmap.
> I.e. the read operation needs to be retried when some other thread has
> made progress (with a write).
> There is no spinning/waiting for other threads to complete. This makes
> the design non-blocking (for readers).
>
> And it has almost no additional overhead because counter and bitmap
> share one 32 bits variable. No additional load/store for reader and
> writer.
>
>
> IIUC, after this change if writer will start inserting the node, reader
> will not find any node with the same hash in cmap because it will check
> only "valid" slots. This breaks the cmap API and could lead to crash.
> Why wouldn't readers find other valid slots with the same hash value?
> It is only the slot that is being updated that is cleared in the valid bitmap
> duing the update. Other valid slots (irrespective of actual hash values) are
> unchanged.
>
> Am I missing something? Do the users of cmap have some other expectations that
> are not obvious from looking at the cmap code?
>
> bucket->nodes[i] is not a single node, but the list of nodes with the same
> hash
> equal to bucket->hashes[i].
> Thanks. I missed this.
>
> ovsrcu_set(>nodes[i].next, node); /* Also atomic. */
> To maintain the correctness of the list, node->next must already have been
> written with the old value of b->nodes[i].next? Which means part of the
> implementation is actually outside of cmap_set_bucket(). That makes it
> difficult
> to change the design, need to modify all callers of cmap_set_bucket() if we
> want
> to do e.g. an atomic enqueue to the list.
>
> cmap_set_bucket() also writes bucket->hashes[i] so this could actually change
> the hash value for existing entries in the nodes[i] list?
>
> Giving a second look at cmap implementation it seems that cmap_set_bucket()
> could only initialize a new slot or move the entire list from one slot to
> another duplicating it. List operations are done manually without blocking
> the slot. It's OK, because list itself is RCU protected. However, I'm still
> not sure that all the cases will be correctly handled. Need to look closer.
I don't think these calls are well-defined and well-documented. Or we wouldn't
have this discussion.
-- Ola
>
>
> You may look at the implementation of
> CMAP_NODE_FOR_EACH/CMAP_FOR_EACH_WITH_HASH
> and read comments to 'struct cmap_node' and cmap.{c,h} in general.
>
> While adding the new node to the list you're restricting access to
> the whole list making it impossible to find any node there.
> I understand that now.
>
>
> Best regards, Ilya Maximets.
>
>
>
--
Ola Liljedahl, System Architect, Arm
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net 1/2] openvswitch: support asymmetric conntrack
Pravin Shelar writes:
> On Fri, Nov 8, 2019 at 1:07 PM Aaron Conole wrote:
>>
>> The openvswitch module shares a common conntrack and NAT infrastructure
>> exposed via netfilter. It's possible that a packet needs both SNAT and
>> DNAT manipulation, due to e.g. tuple collision. Netfilter can support
>> this because it runs through the NAT table twice - once on ingress and
>> again after egress. The openvswitch module doesn't have such capability.
>>
>> Like netfilter hook infrastructure, we should run through NAT twice to
>> keep the symmetry.
>>
>> Fixes: 05752523e565 ("openvswitch: Interface with NAT.")
>> Signed-off-by: Aaron Conole
>
> The patch looks ok. But I am not able apply it. can you fix the encoding.
Hrrm. I didn't make any special changes (just used git send-email). I
will look at spinning a second patch.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] rhel: Fix ovs-kmod-manage.sh that may create invalid soft links
On Mon, Nov 18, 2019 at 12:19:26PM -0800, Yi-Hung Wei wrote:
> On Mon, Nov 18, 2019 at 12:06 PM Yifeng Sun wrote:
> >
> > Current code iterates every kernel under '/lib/modules' for a matched
> > version. As a result, this script may create invalid soft links if the
> > matched kernel doesn't have openvswitch-kmod RPM installed.
> >
> > This patch fixes it.
> >
> > VMWare-BZ: #2257534
> >
> > Fixes: c3570519 ("rhel: add 4.4 kernel in kmod build with mulitple
> > versions, fedora")
> > Signed-off-by: Yifeng Sun
>
> LGTM,
>
> Acked-by: Yi-Hung Wei
Thanks, I applied to master.
William
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] rhel: Fix ovs-kmod-manage.sh that may create invalid soft links
On Mon, Nov 18, 2019 at 12:06 PM Yifeng Sun wrote:
>
> Current code iterates every kernel under '/lib/modules' for a matched
> version. As a result, this script may create invalid soft links if the
> matched kernel doesn't have openvswitch-kmod RPM installed.
>
> This patch fixes it.
>
> VMWare-BZ: #2257534
>
> Fixes: c3570519 ("rhel: add 4.4 kernel in kmod build with mulitple versions,
> fedora")
> Signed-off-by: Yifeng Sun
LGTM,
Acked-by: Yi-Hung Wei
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] rhel: Fix ovs-kmod-manage.sh that may create invalid soft links
Current code iterates every kernel under '/lib/modules' for a matched
version. As a result, this script may create invalid soft links if the
matched kernel doesn't have openvswitch-kmod RPM installed.
This patch fixes it.
VMWare-BZ: #2257534
Fixes: c3570519 ("rhel: add 4.4 kernel in kmod build with mulitple versions,
fedora")
Signed-off-by: Yifeng Sun
---
v1->v2: Added fix tag, thanks YiHung.
rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh
b/rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh
index a643b55ff0f8..a252b391ecba 100644
--- a/rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh
+++ b/rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh
@@ -157,7 +157,7 @@ fi
#$kmod_high_ver"
found_match=false
-for kname in `ls -d /lib/modules/*`
+for kname in $kversion;
do
IFS='.\|-' read -r -a pkg_ver_nums <<<"${kname}"
pkg_ver=${pkg_ver_nums[$ver_offset]}
@@ -184,14 +184,14 @@ if [ "$found_match" = "false" ]; then
exit 1
fi
-if [ "$requested_kernel" != "/lib/modules/$current_kernel" ]; then
+if [ "$requested_kernel" != "$current_kernel" ]; then
if [ ! -d /lib/modules/$current_kernel/weak-updates/openvswitch ]; then
mkdir -p /lib/modules/$current_kernel/weak-updates
mkdir -p /lib/modules/$current_kernel/weak-updates/openvswitch
fi
for m in openvswitch vport-gre vport-stt vport-geneve \
vport-lisp vport-vxlan; do
-ln -f -s $requested_kernel/extra/openvswitch/$m.ko \
+ln -f -s /lib/modules/$requested_kernel/extra/openvswitch/$m.ko \
/lib/modules/$current_kernel/weak-updates/openvswitch/$m.ko
done
else
--
2.7.4
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] rhel: Fix ovs-kmod-manage.sh that may create invalid soft links
Will do, thanks! On Mon, Nov 18, 2019 at 11:35 AM Yi-Hung Wei wrote: > > On Mon, Nov 18, 2019 at 11:24 AM Yifeng Sun wrote: > > > > Current code iterates every kernel under '/lib/modules' for a matched > > version. As a result, this script may create invalid soft links if the > > matched kernel doesn't have openvswitch-kmod RPM installed. > > > > This patch fixes it. > > > > VMWare-BZ: #2257534 > > > > Signed-off-by: Yifeng Sun > > --- > > Thanks Yifeng for the fix. This patch fixes the issue where RHEL has > been upgraded, and the script may create weak-update symbolic link to > the old kernel's directory. > > Can you add a fix tag to this patch? Otherwise, this patch looks good to me. > > Acked-by: Yi-Hung Wei > > Thanks, > > -Yi-Hung ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5 ovn 1/4] ovn-controller: Refactor I-P engine_run() tracking.
On Mon, Nov 18, 2019 at 6:07 AM Dumitru Ceara wrote:
>
> This commit simplifies the logic of calling engine_run and
engine_need_run in
> order to reduce the number of external variables required to track the
result
> of the last engine execution.
>
> The engine code is also refactored a bit and the engine_run() function is
> split in different functions that handle computing/recomputing a node.
>
> Signed-off-by: Dumitru Ceara
> ---
> controller/ovn-controller.c | 33 ++-
> lib/inc-proc-eng.c | 124
+--
> lib/inc-proc-eng.h |7 ++
> 3 files changed, 107 insertions(+), 57 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 27cb488..c56190f 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1942,7 +1942,6 @@ main(int argc, char *argv[])
> _pkt);
>
> uint64_t engine_run_id = 0;
> -uint64_t old_engine_run_id = 0;
> bool engine_run_done = true;
>
> unsigned int ovs_cond_seqno = UINT_MAX;
> @@ -1952,10 +1951,11 @@ main(int argc, char *argv[])
> exiting = false;
> restart = false;
> while (!exiting) {
> +engine_run_id++;
> +
> update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl);
> update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl));
>
ofctrl_set_probe_interval(get_ofctrl_probe_interval(ovs_idl_loop.idl));
> -old_engine_run_id = engine_run_id;
>
> struct ovsdb_idl_txn *ovs_idl_txn =
ovsdb_idl_loop_run(_idl_loop);
> unsigned int new_ovs_cond_seqno
> @@ -2047,12 +2047,12 @@ main(int argc, char *argv[])
> if (engine_run_done) {
> engine_set_abort_recompute(true);
> engine_run_done =
engine_run(_flow_output,
> -
++engine_run_id);
> +
engine_run_id);
> }
> } else {
> engine_set_abort_recompute(false);
> engine_run_done = true;
> -engine_run(_flow_output, ++engine_run_id);
> +engine_run(_flow_output, engine_run_id);
> }
> }
> stopwatch_stop(CONTROLLER_LOOP_STOPWATCH_NAME,
> @@ -2097,17 +2097,20 @@ main(int argc, char *argv[])
> }
>
> }
> -if (old_engine_run_id == engine_run_id || !engine_run_done) {
> -if (!engine_run_done ||
engine_need_run(_flow_output)) {
> -VLOG_DBG("engine did not run, force recompute next
time: "
> - "br_int %p, chassis %p", br_int, chassis);
> -engine_set_force_recompute(true);
> -poll_immediate_wake();
> -} else {
> -VLOG_DBG("engine did not run, and it was not needed"
> - " either: br_int %p, chassis %p",
> - br_int, chassis);
> -}
> +if (engine_need_run(_flow_output, engine_run_id)) {
> +VLOG_DBG("engine did not run, force recompute next time:
"
> +"br_int %p, chassis %p", br_int, chassis);
> +engine_set_force_recompute(true);
> +poll_immediate_wake();
> +} else if (!engine_run_done) {
> +VLOG_DBG("engine was aborted, force recompute next time:
"
> + "br_int %p, chassis %p", br_int, chassis);
> +engine_set_force_recompute(true);
> +poll_immediate_wake();
> +} else if (!engine_has_run(_flow_output, engine_run_id)) {
> +VLOG_DBG("engine did not run, and it was not needed"
> + " either: br_int %p, chassis %p",
> + br_int, chassis);
> } else {
> engine_set_force_recompute(false);
> }
> diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c
> index 1064a08..8a085e2 100644
> --- a/lib/inc-proc-eng.c
> +++ b/lib/inc-proc-eng.c
> @@ -129,14 +129,72 @@ engine_ovsdb_node_add_index(struct engine_node
*node, const char *name,
> }
>
> bool
> -engine_run(struct engine_node *node, uint64_t run_id)
> +engine_has_run(struct engine_node *node, uint64_t run_id)
> +{
> +return node->run_id == run_id;
> +}
> +
> +/* Do a full recompute (or at least try). If we're not allowed then
> + * mark the node as "aborted".
> + */
> +static bool
> +engine_recompute(struct engine_node *node, bool forced, bool allowed)
> +{
> +VLOG_DBG("node: %s, recompute (%s)", node->name,
> + forced ? "forced" : "triggered");
> +
> +if (!allowed) {
> +VLOG_DBG("node: %s, recompute aborted", node->name);
> +return false;
> +}
> +
> +node->run(node);
> +
Re: [ovs-dev] [PATCH v3] travis: support ppc64le builds
On 2019-11-12 11:30, Ilya Maximets wrote: On 12.11.2019 18:57, dwilder wrote: On 2019-11-08 14:52, Ilya Maximets wrote: On 06.11.2019 20:20, David Wilder wrote: Add support for travis-ci ppc64le builds. - Updated matrix in .travis.yml to include an arch: ppc64le build. - Move package install needed for 32bit builds to .travis/linux-prepare.sh. To keep the total build time at an acceptable level only a single build job is included in the matrix for ppc64le. A build report example can be found here [1] [0] https://urldefense.proofpoint.com/v2/url?u=http-3A__travis-2Dci.org_=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=7ndxyKjH9UrBD68Us2WP1wI4BwEBQbeAyz8i_vwCCaI=_UtshYcJsj4Pt3b9hfgEiyaEIT3j9gPEIgmBatCEqCo=r0fBOs-21CKcV4kyZGnzh3fcjrpR8caYSl8K2i1St54= [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__travis-2Dci.org_djlwilder_ovs_builds_607851729=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=7ndxyKjH9UrBD68Us2WP1wI4BwEBQbeAyz8i_vwCCaI=_UtshYcJsj4Pt3b9hfgEiyaEIT3j9gPEIgmBatCEqCo=7t2rzVasH7Xq_R7jWkWZO9rkgm4KHMH-WavBzCRbF74= Signed-off-by: David Wilder --- Addressed review comments: - Cleaned up linux-prepare.sh (v2) - Switch from os: linux-ppc64le to arch: ppc64le (v3) What a wonderful world of undocumented features. :) Anyway, I just tried this patch and it fails for me because of missing pip: https://urldefense.proofpoint.com/v2/url?u=https-3A__travis-2Dci.org_igsilya_ovs_jobs_609402867=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=7ndxyKjH9UrBD68Us2WP1wI4BwEBQbeAyz8i_vwCCaI=_UtshYcJsj4Pt3b9hfgEiyaEIT3j9gPEIgmBatCEqCo=PF1oO_KkZFd_RRKToj6UBN2t2YhvTVE5XnVD1GF9u60= pip install --disable-pip-version-check --user six flake8 hacking ./.travis/linux-prepare.sh: line 15: pip: command not found Restarting the job doesn't help. I'm wondering what is the base image and who controls preinstalled software? Maybe it makes sense to hold on until travis will start supporting ppc64le officially? Best regards, Ilya Maximets. Unfortunately it not just ppc the arm64 image has the same issue :) I added a arm64 build on this attempt. https://urldefense.proofpoint.com/v2/url?u=https-3A__travis-2Dci.org_djlwilder_ovs_builds_610517731=DwIDaQ=jf_iaSHvJObTbx-siA1ZOg=7ndxyKjH9UrBD68Us2WP1wI4BwEBQbeAyz8i_vwCCaI=rVTSRTh9jExG_mX8boqA-OQcZkWmbO2g7TjgtUe6jws=sCvTr8MwXrqa7AEOs60tuqnquBqbRKlp_7-WacGzJWc= I will attempt to adjust the package list. I think, you could just report this to TravisCI support and see what they will answer. It'll be much better if they will just update their images. And the good news that ppc64le is officially supported now: https://urldefense.proofpoint.com/v2/url?u=https-3A__blog.travis-2Dci.com_2019-2D11-2D12-2Dmulti-2Dcpu-2Darchitecture-2Dibm-2Dpower-2Dibm-2Dz=DwIDaQ=jf_iaSHvJObTbx-siA1ZOg=7ndxyKjH9UrBD68Us2WP1wI4BwEBQbeAyz8i_vwCCaI=rVTSRTh9jExG_mX8boqA-OQcZkWmbO2g7TjgtUe6jws=CRYw9o6At3rFT_gd_5r1Uw07mv_SEPvQ6LgBLfMyqgg= Best regards, Ilya Maximets. Travis-ci.org has resolved the issue with pip not installed in the ppc64le and arm64 images (1) I re-tested my v3 patch against the head of master (2). It is working well. (1) https://travis-ci.community/t/pip-is-not-installed-in-ppc64le-and-arm64-images/5902 (2) https://travis-ci.org/djlwilder/ovs/builds/613638217 Regards, David Wilder ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] rhel: Fix ovs-kmod-manage.sh that may create invalid soft links
On Mon, Nov 18, 2019 at 11:24 AM Yifeng Sun wrote: > > Current code iterates every kernel under '/lib/modules' for a matched > version. As a result, this script may create invalid soft links if the > matched kernel doesn't have openvswitch-kmod RPM installed. > > This patch fixes it. > > VMWare-BZ: #2257534 > > Signed-off-by: Yifeng Sun > --- Thanks Yifeng for the fix. This patch fixes the issue where RHEL has been upgraded, and the script may create weak-update symbolic link to the old kernel's directory. Can you add a fix tag to this patch? Otherwise, this patch looks good to me. Acked-by: Yi-Hung Wei Thanks, -Yi-Hung ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] rhel: Fix ovs-kmod-manage.sh that may create invalid soft links
Current code iterates every kernel under '/lib/modules' for a matched
version. As a result, this script may create invalid soft links if the
matched kernel doesn't have openvswitch-kmod RPM installed.
This patch fixes it.
VMWare-BZ: #2257534
Signed-off-by: Yifeng Sun
---
rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh
b/rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh
index a643b55ff0f8..a252b391ecba 100644
--- a/rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh
+++ b/rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh
@@ -157,7 +157,7 @@ fi
#$kmod_high_ver"
found_match=false
-for kname in `ls -d /lib/modules/*`
+for kname in $kversion;
do
IFS='.\|-' read -r -a pkg_ver_nums <<<"${kname}"
pkg_ver=${pkg_ver_nums[$ver_offset]}
@@ -184,14 +184,14 @@ if [ "$found_match" = "false" ]; then
exit 1
fi
-if [ "$requested_kernel" != "/lib/modules/$current_kernel" ]; then
+if [ "$requested_kernel" != "$current_kernel" ]; then
if [ ! -d /lib/modules/$current_kernel/weak-updates/openvswitch ]; then
mkdir -p /lib/modules/$current_kernel/weak-updates
mkdir -p /lib/modules/$current_kernel/weak-updates/openvswitch
fi
for m in openvswitch vport-gre vport-stt vport-geneve \
vport-lisp vport-vxlan; do
-ln -f -s $requested_kernel/extra/openvswitch/$m.ko \
+ln -f -s /lib/modules/$requested_kernel/extra/openvswitch/$m.ko \
/lib/modules/$current_kernel/weak-updates/openvswitch/$m.ko
done
else
--
2.7.4
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 2/2] cmap: non-blocking cmap lookup
On 18.11.2019 17:24, Ola Liljedahl wrote:
> On Mon, 2019-11-18 at 17:06 +0100, Ilya Maximets wrote:
>> On 18.11.2019 16:55, Ola Liljedahl wrote:
>>
>> On Mon, 2019-11-18 at 16:45 +0100, Ilya Maximets wrote:
>>
>> On 18.11.2019 3:45, Yanqin Wei wrote:
>>
>> Currently cmap_bucket is protected by a counter. Cmap reader will be
>> blocked until the counter becomes even, writer increments it by 1 to be
>> odd, and after slot update, increments by 1 again to become even. If the
>> writer is pending or scheduled out during the writer course, the reader
>> will be blocked.
>>
>> This patch introduces a bitmap as guard variable for (hash,node) pair.
>> Writer need set slot valid bit to 0 before each change. And after change,
>> writer sets valid bit to 1 and increments counter. Thus, reader can ensure
>> slot consistency by only scanning valid slot and then checking counter+bitmap
>> does not change while examining the bucket.
>>
>> The only time a reader has to retry the read operation for single bucket
>> is when
>> 1)a writer clears a bit in the valid bitmap between a reader's first and
>> second read of the counter+bitmap.
>> 2)a writer increments the counter between a reader's first and second
>> read of counter+bitmap.
>> I.e. the read operation needs to be retried when some other thread has
>> made progress (with a write).
>> There is no spinning/waiting for other threads to complete. This makes
>> the design non-blocking (for readers).
>>
>> And it has almost no additional overhead because counter and bitmap
>> share one 32 bits variable. No additional load/store for reader and
>> writer.
>>
>>
>> IIUC, after this change if writer will start inserting the node, reader
>> will not find any node with the same hash in cmap because it will check
>> only "valid" slots. This breaks the cmap API and could lead to crash.
>> Why wouldn't readers find other valid slots with the same hash value?
>> It is only the slot that is being updated that is cleared in the valid bitmap
>> duing the update. Other valid slots (irrespective of actual hash values) are
>> unchanged.
>>
>> Am I missing something? Do the users of cmap have some other expectations
>> that
>> are not obvious from looking at the cmap code?
>>
>> bucket->nodes[i] is not a single node, but the list of nodes with the same
>> hash
>> equal to bucket->hashes[i].
> Thanks. I missed this.
>
> ovsrcu_set(>nodes[i].next, node); /* Also atomic. */
> To maintain the correctness of the list, node->next must already have been
> written with the old value of b->nodes[i].next? Which means part of the
> implementation is actually outside of cmap_set_bucket(). That makes it
> difficult
> to change the design, need to modify all callers of cmap_set_bucket() if we
> want
> to do e.g. an atomic enqueue to the list.
>
> cmap_set_bucket() also writes bucket->hashes[i] so this could actually change
> the hash value for existing entries in the nodes[i] list?
Giving a second look at cmap implementation it seems that cmap_set_bucket()
could only initialize a new slot or move the entire list from one slot to
another duplicating it. List operations are done manually without blocking
the slot. It's OK, because list itself is RCU protected. However, I'm still
not sure that all the cases will be correctly handled. Need to look closer.
>
>>
>> You may look at the implementation of
>> CMAP_NODE_FOR_EACH/CMAP_FOR_EACH_WITH_HASH
>> and read comments to 'struct cmap_node' and cmap.{c,h} in general.
>>
>> While adding the new node to the list you're restricting access to
>> the whole list making it impossible to find any node there.
> I understand that now.
>
>>
>> Best regards, Ilya Maximets.
>>
>>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 2/2] ofproto: Do not delete datapath flows on exit by default.
Commit e96a5c24e853 ("upcall: Remove datapath flows when setting
n-threads.") caused OVS to delete datapath flows when it exits through
any graceful means. This is not necessarily desirable, especially when
OVS is being stopped as part of an upgrade. This commit changes OVS so
that it only removes datapath flows when requested, via "ovs-appctl
exit --cleanup".
I've only tested this in the OVS sandbox.
Signed-off-by: Ben Pfaff
---
ofproto/ofproto-dpif-upcall.c | 26 --
ofproto/ofproto.c | 8
2 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 9b3b4470b0af..b25d51279678 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -331,7 +331,7 @@ static size_t recv_upcalls(struct handler *);
static int process_upcall(struct udpif *, struct upcall *,
struct ofpbuf *odp_actions, struct flow_wildcards *);
static void handle_upcalls(struct udpif *, struct upcall *, size_t n_upcalls);
-static void udpif_stop_threads(struct udpif *);
+static void udpif_stop_threads(struct udpif *, bool delete_flows);
static void udpif_start_threads(struct udpif *, size_t n_handlers,
size_t n_revalidators);
static void udpif_pause_revalidators(struct udpif *);
@@ -482,7 +482,7 @@ udpif_run(struct udpif *udpif)
void
udpif_destroy(struct udpif *udpif)
{
-udpif_stop_threads(udpif);
+udpif_stop_threads(udpif, false);
dpif_register_dp_purge_cb(udpif->dpif, NULL, udpif);
dpif_register_upcall_cb(udpif->dpif, NULL, udpif);
@@ -503,9 +503,15 @@ udpif_destroy(struct udpif *udpif)
free(udpif);
}
-/* Stops the handler and revalidator threads. */
+/* Stops the handler and revalidator threads.
+ *
+ * If 'delete_flows' is true, we delete ukeys and delete all flows from the
+ * datapath. Otherwise, we end up double-counting stats for flows that remain
+ * in the datapath. If 'delete_flows' is false, we skip this step. This is
+ * appropriate if OVS is about to exit anyway and it is desirable to let
+ * existing network connections continue being forwarded afterward. */
static void
-udpif_stop_threads(struct udpif *udpif)
+udpif_stop_threads(struct udpif *udpif, bool delete_flows)
{
if (udpif && (udpif->n_handlers != 0 || udpif->n_revalidators != 0)) {
size_t i;
@@ -525,10 +531,10 @@ udpif_stop_threads(struct udpif *udpif)
dpif_disable_upcall(udpif->dpif);
ovsrcu_quiesce_end();
-/* Delete ukeys, and delete all flows from the datapath to prevent
- * double-counting stats. */
-for (i = 0; i < udpif->n_revalidators; i++) {
-revalidator_purge(>revalidators[i]);
+if (delete_flows) {
+for (i = 0; i < udpif->n_revalidators; i++) {
+revalidator_purge(>revalidators[i]);
+}
}
latch_poll(>exit_latch);
@@ -626,7 +632,7 @@ udpif_set_threads(struct udpif *udpif, size_t n_handlers_,
if (udpif->n_handlers != n_handlers_
|| udpif->n_revalidators != n_revalidators_) {
-udpif_stop_threads(udpif);
+udpif_stop_threads(udpif, true);
}
if (!udpif->handlers && !udpif->revalidators) {
@@ -680,7 +686,7 @@ udpif_flush(struct udpif *udpif)
size_t n_handlers_ = udpif->n_handlers;
size_t n_revalidators_ = udpif->n_revalidators;
-udpif_stop_threads(udpif);
+udpif_stop_threads(udpif, true);
dpif_flow_flush(udpif->dpif);
udpif_start_threads(udpif, n_handlers_, n_revalidators_);
}
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 3aaa45a9b3af..23117c7f304e 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1589,13 +1589,13 @@ ofproto_rule_delete(struct ofproto *ofproto, struct
rule *rule)
}
static void
-ofproto_flush__(struct ofproto *ofproto)
+ofproto_flush__(struct ofproto *ofproto, bool del)
OVS_EXCLUDED(ofproto_mutex)
{
struct oftable *table;
/* This will flush all datapath flows. */
-if (ofproto->ofproto_class->flush) {
+if (del && ofproto->ofproto_class->flush) {
ofproto->ofproto_class->flush(ofproto);
}
@@ -1698,7 +1698,7 @@ ofproto_destroy(struct ofproto *p, bool del)
return;
}
-ofproto_flush__(p);
+ofproto_flush__(p, del);
HMAP_FOR_EACH_SAFE (ofport, next_ofport, hmap_node, >ports) {
ofport_destroy(ofport, del);
}
@@ -2276,7 +2276,7 @@ void
ofproto_flush_flows(struct ofproto *ofproto)
{
COVERAGE_INC(ofproto_flush);
-ofproto_flush__(ofproto);
+ofproto_flush__(ofproto, false);
connmgr_flushed(ofproto->connmgr);
}
--
2.21.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 1/2] ofproto-dpif-upcall: Get rid of udpif_synchronize().
RCU provides the semantics we want from udpif_synchronize() and it
should be much more lightweight than killing and restarting all the
upcall threads. It looks like udpif_synchronize() was written before
the OVS tree had RCU support, which is probably why we didn't use it
here from the beginning. So we can just change udpif_synchronize()
to a single ovsrcu_synchronize() call.
However, udpif_synchronize() only has a single caller, which calls
ovsrcu_synchronize() anyway just beforehand, via xlate_txn_commit().
So we can get rid of udpif_synchronize() entirely, which this patch
does.
As a side effect, this eliminates one reason why terminating OVS cleanly
clears the datapath flow table. An upcoming patch will eliminate
other reasons.
Signed-off-by: Ben Pfaff
---
ofproto/ofproto-dpif-upcall.c | 17 -
ofproto/ofproto-dpif-upcall.h | 1 -
ofproto/ofproto-dpif-xlate.c | 14 +-
ofproto/ofproto-dpif.c| 4
4 files changed, 9 insertions(+), 27 deletions(-)
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 657aa7f79208..9b3b4470b0af 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -643,23 +643,6 @@ udpif_set_threads(struct udpif *udpif, size_t n_handlers_,
}
}
-/* Waits for all ongoing upcall translations to complete. This ensures that
- * there are no transient references to any removed ofprotos (or other
- * objects). In particular, this should be called after an ofproto is removed
- * (e.g. via xlate_remove_ofproto()) but before it is destroyed. */
-void
-udpif_synchronize(struct udpif *udpif)
-{
-/* This is stronger than necessary. It would be sufficient to ensure
- * (somehow) that each handler and revalidator thread had passed through
- * its main loop once. */
-size_t n_handlers_ = udpif->n_handlers;
-size_t n_revalidators_ = udpif->n_revalidators;
-
-udpif_stop_threads(udpif);
-udpif_start_threads(udpif, n_handlers_, n_revalidators_);
-}
-
/* Notifies 'udpif' that something changed which may render previous
* xlate_actions() results invalid. */
void
diff --git a/ofproto/ofproto-dpif-upcall.h b/ofproto/ofproto-dpif-upcall.h
index cef1d34198d6..693107ae56c1 100644
--- a/ofproto/ofproto-dpif-upcall.h
+++ b/ofproto/ofproto-dpif-upcall.h
@@ -33,7 +33,6 @@ struct udpif *udpif_create(struct dpif_backer *, struct dpif
*);
void udpif_run(struct udpif *udpif);
void udpif_set_threads(struct udpif *, size_t n_handlers,
size_t n_revalidators);
-void udpif_synchronize(struct udpif *);
void udpif_destroy(struct udpif *);
void udpif_revalidate(struct udpif *);
void udpif_get_memory_usage(struct udpif *, struct simap *usage);
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index f92cb62c80ce..e375dc1b085f 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1165,11 +1165,15 @@ xlate_xport_copy(struct xbridge *xbridge, struct
xbundle *xbundle,
*
* A sample workflow:
*
- * xlate_txn_start();
- * ...
- * edit_xlate_configuration();
- * ...
- * xlate_txn_commit(); */
+ * xlate_txn_start();
+ * ...
+ * edit_xlate_configuration();
+ * ...
+ * xlate_txn_commit();
+ *
+ * The ovsrcu_synchronize() call here also ensures that the upcall threads
+ * retain no references to anything in the previous configuration.
+ */
void
xlate_txn_commit(void)
{
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index c35ec3e61592..d56629b005ea 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1706,10 +1706,6 @@ destruct(struct ofproto *ofproto_, bool del)
xlate_remove_ofproto(ofproto);
xlate_txn_commit();
-/* Ensure that the upcall processing threads have no remaining references
- * to the ofproto or anything in it. */
-udpif_synchronize(ofproto->backer->udpif);
-
hmap_remove(_ofproto_dpifs_by_name,
>all_ofproto_dpifs_by_name_node);
hmap_remove(_ofproto_dpifs_by_uuid,
--
2.21.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] Cierre de Inscripciones
27 de Noviembre | Horario de 10:00 a 17:00 hrs. | (hora del centro de México) - Rotación Cero - Webinar en Línea La rotación de personal es un fenómeno que afecta a la mayoría de las organizaciones y merma el rendimiento de los colaboradores. Innova Learn propone este webinar en el que se revisarán las bases teóricas que apoyen a los participantes en la implementación de tácticas que subsanen esta problemática que influye en la cultura organizacional de la empresa. Presentaremos los fundamentos para identificar el impacto que la rotación de personal tiene en nuestra empresa así como de las estrategias a aplicar para sobrellevarla. Solicita información respondiendo a este correo con la palabra CERO, junto con los siguientes datos: Nombre: Correo electrónico: Número telefónico: Dirigido a: Personal del área de RRHH, Reclutamiento y Selección de personal, Desarrollo Organizacional. Directores, gerentes y jefes responsables de la administración de personal. Números de Atención: (045) 55 15 54 66 30 - (045) 55 85567293 - (045) 5530167085 En caso de que haya recibido este correo sin haberlo solicitado o si desea dejar de recibir nuestra promoción favor de responder con la palabra baja o enviar un correo a bajas@ innovalearn.net ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 2/2] cmap: non-blocking cmap lookup
On Mon, 2019-11-18 at 16:45 +0100, Ilya Maximets wrote: > On 18.11.2019 3:45, Yanqin Wei wrote: > > Currently cmap_bucket is protected by a counter. Cmap reader will be > blocked until the counter becomes even, writer increments it by 1 to be > odd, and after slot update, increments by 1 again to become even. If the > writer is pending or scheduled out during the writer course, the reader > will be blocked. > > This patch introduces a bitmap as guard variable for (hash,node) pair. > Writer need set slot valid bit to 0 before each change. And after change, > writer sets valid bit to 1 and increments counter. Thus, reader can ensure > slot consistency by only scanning valid slot and then checking counter+bitmap > does not change while examining the bucket. > > The only time a reader has to retry the read operation for single bucket > is when > 1)a writer clears a bit in the valid bitmap between a reader's first and > second read of the counter+bitmap. > 2)a writer increments the counter between a reader's first and second > read of counter+bitmap. > I.e. the read operation needs to be retried when some other thread has > made progress (with a write). > There is no spinning/waiting for other threads to complete. This makes > the design non-blocking (for readers). > > And it has almost no additional overhead because counter and bitmap > share one 32 bits variable. No additional load/store for reader and > writer. > > > IIUC, after this change if writer will start inserting the node, reader > will not find any node with the same hash in cmap because it will check > only "valid" slots. This breaks the cmap API and could lead to crash. Why wouldn't readers find other valid slots with the same hash value? It is only the slot that is being updated that is cleared in the valid bitmap duing the update. Other valid slots (irrespective of actual hash values) are unchanged. Am I missing something? Do the users of cmap have some other expectations that are not obvious from looking at the cmap code? -- Ola > > Best regards, Ilya Maximets. > > -- Ola Liljedahl, System Architect, Arm ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 2/2] cmap: non-blocking cmap lookup
On Mon, 2019-11-18 at 17:06 +0100, Ilya Maximets wrote:
> On 18.11.2019 16:55, Ola Liljedahl wrote:
>
> On Mon, 2019-11-18 at 16:45 +0100, Ilya Maximets wrote:
>
> On 18.11.2019 3:45, Yanqin Wei wrote:
>
> Currently cmap_bucket is protected by a counter. Cmap reader will be
> blocked until the counter becomes even, writer increments it by 1 to be
> odd, and after slot update, increments by 1 again to become even. If the
> writer is pending or scheduled out during the writer course, the reader
> will be blocked.
>
> This patch introduces a bitmap as guard variable for (hash,node) pair.
> Writer need set slot valid bit to 0 before each change. And after change,
> writer sets valid bit to 1 and increments counter. Thus, reader can ensure
> slot consistency by only scanning valid slot and then checking counter+bitmap
> does not change while examining the bucket.
>
> The only time a reader has to retry the read operation for single bucket
> is when
> 1)a writer clears a bit in the valid bitmap between a reader's first and
> second read of the counter+bitmap.
> 2)a writer increments the counter between a reader's first and second
> read of counter+bitmap.
> I.e. the read operation needs to be retried when some other thread has
> made progress (with a write).
> There is no spinning/waiting for other threads to complete. This makes
> the design non-blocking (for readers).
>
> And it has almost no additional overhead because counter and bitmap
> share one 32 bits variable. No additional load/store for reader and
> writer.
>
>
> IIUC, after this change if writer will start inserting the node, reader
> will not find any node with the same hash in cmap because it will check
> only "valid" slots. This breaks the cmap API and could lead to crash.
> Why wouldn't readers find other valid slots with the same hash value?
> It is only the slot that is being updated that is cleared in the valid bitmap
> duing the update. Other valid slots (irrespective of actual hash values) are
> unchanged.
>
> Am I missing something? Do the users of cmap have some other expectations that
> are not obvious from looking at the cmap code?
>
> bucket->nodes[i] is not a single node, but the list of nodes with the same
> hash
> equal to bucket->hashes[i].
Thanks. I missed this.
ovsrcu_set(>nodes[i].next, node); /* Also atomic. */
To maintain the correctness of the list, node->next must already have been
written with the old value of b->nodes[i].next? Which means part of the
implementation is actually outside of cmap_set_bucket(). That makes it difficult
to change the design, need to modify all callers of cmap_set_bucket() if we want
to do e.g. an atomic enqueue to the list.
cmap_set_bucket() also writes bucket->hashes[i] so this could actually change
the hash value for existing entries in the nodes[i] list?
>
> You may look at the implementation of
> CMAP_NODE_FOR_EACH/CMAP_FOR_EACH_WITH_HASH
> and read comments to 'struct cmap_node' and cmap.{c,h} in general.
>
> While adding the new node to the list you're restricting access to
> the whole list making it impossible to find any node there.
I understand that now.
>
> Best regards, Ilya Maximets.
>
>
--
Ola Liljedahl, System Architect, Arm
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] Comunicación efectiva y empática del coach
26 de Noviembre | Horario de 10:00 a 17:00 hrs. | (hora del centro de México) - Coaching y liderazgo efectivo. - El líder coach es un estilo muy completo para utilizar cuando queremos capacitar a otros y conseguir resultados sólidos a mediano y largo plazo. Este webinar tiene como finalidad llevar a los participantes a ser capaces de intervenir positivamente en otros individuos en funciones de liderazgo, comprendiendo diversos estilos y aplicando en la práctica acciones que permitan mejorar la toma de decisiones para alcanzar los objetivos de su organización. Solicita información respondiendo a este correo con la palabra Coaching, junto con los siguientes datos: Nombre: Correo electrónico: Número telefónico: Números de Atención: (045) 55 15 54 66 30 - (045) 55 85567293 - (045) 5530167085 En caso de que haya recibido este correo sin haberlo solicitado o si desea dejar de recibir nuestra promoción favor de responder con la palabra baja o enviar un correo a bajas@ innovalearn.net ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next] openvswitch: add TTL decrement action
On Tue, Nov 12, 2019 at 04:46:12PM +0100, Matteo Croce wrote:
> On Tue, Nov 12, 2019 at 4:00 PM Simon Horman
> wrote:
> >
> > On Tue, Nov 12, 2019 at 11:25:18AM +0100, Matteo Croce wrote:
> > > New action to decrement TTL instead of setting it to a fixed value.
> > > This action will decrement the TTL and, in case of expired TTL, send the
> > > packet to userspace via output_userspace() to take care of it.
> > >
> > > Supports both IPv4 and IPv6 via the ttl and hop_limit fields,
> > > respectively.
> > >
> >
> > Usually OVS achieves this behaviour by matching on the TTL and
> > setting it to the desired value, pre-calculated as TTL -1.
> > With that in mind could you explain the motivation for this
> > change?
> >
>
> Hi,
>
> the problem is that OVS creates a flow for each ttl it see. I can let
> vswitchd create 255 flows with like this:
>
> $ for i in {2..255}; do ping 192.168.0.2 -t $i -c1 -w1 &>/dev/null & done
> $ ovs-dpctl dump-flows |fgrep -c 'set(ipv4(ttl'
> 255
Sure, you can easily invent a situation. In real traffic there's not
usually such a variety of TTLs for a flow that matches on the number of
fields that OVS usually needs to match. Do you see a real problem given
actual traffic in practice?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 2/2] cmap: non-blocking cmap lookup
On 18.11.2019 16:55, Ola Liljedahl wrote:
> On Mon, 2019-11-18 at 16:45 +0100, Ilya Maximets wrote:
>> On 18.11.2019 3:45, Yanqin Wei wrote:
>>
>> Currently cmap_bucket is protected by a counter. Cmap reader will be
>> blocked until the counter becomes even, writer increments it by 1 to be
>> odd, and after slot update, increments by 1 again to become even. If the
>> writer is pending or scheduled out during the writer course, the reader
>> will be blocked.
>>
>> This patch introduces a bitmap as guard variable for (hash,node) pair.
>> Writer need set slot valid bit to 0 before each change. And after change,
>> writer sets valid bit to 1 and increments counter. Thus, reader can ensure
>> slot consistency by only scanning valid slot and then checking counter+bitmap
>> does not change while examining the bucket.
>>
>> The only time a reader has to retry the read operation for single bucket
>> is when
>> 1)a writer clears a bit in the valid bitmap between a reader's first and
>> second read of the counter+bitmap.
>> 2)a writer increments the counter between a reader's first and second
>> read of counter+bitmap.
>> I.e. the read operation needs to be retried when some other thread has
>> made progress (with a write).
>> There is no spinning/waiting for other threads to complete. This makes
>> the design non-blocking (for readers).
>>
>> And it has almost no additional overhead because counter and bitmap
>> share one 32 bits variable. No additional load/store for reader and
>> writer.
>>
>>
>> IIUC, after this change if writer will start inserting the node, reader
>> will not find any node with the same hash in cmap because it will check
>> only "valid" slots. This breaks the cmap API and could lead to crash.
> Why wouldn't readers find other valid slots with the same hash value?
> It is only the slot that is being updated that is cleared in the valid bitmap
> duing the update. Other valid slots (irrespective of actual hash values) are
> unchanged.
>
> Am I missing something? Do the users of cmap have some other expectations that
> are not obvious from looking at the cmap code?
bucket->nodes[i] is not a single node, but the list of nodes with the same hash
equal to bucket->hashes[i].
You may look at the implementation of CMAP_NODE_FOR_EACH/CMAP_FOR_EACH_WITH_HASH
and read comments to 'struct cmap_node' and cmap.{c,h} in general.
While adding the new node to the list you're restricting access to
the whole list making it impossible to find any node there.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 2/2] cmap: non-blocking cmap lookup
On 18.11.2019 3:45, Yanqin Wei wrote: > Currently cmap_bucket is protected by a counter. Cmap reader will be > blocked until the counter becomes even, writer increments it by 1 to be > odd, and after slot update, increments by 1 again to become even. If the > writer is pending or scheduled out during the writer course, the reader > will be blocked. > > This patch introduces a bitmap as guard variable for (hash,node) pair. > Writer need set slot valid bit to 0 before each change. And after change, > writer sets valid bit to 1 and increments counter. Thus, reader can ensure > slot consistency by only scanning valid slot and then checking counter+bitmap > does not change while examining the bucket. > > The only time a reader has to retry the read operation for single bucket > is when > 1)a writer clears a bit in the valid bitmap between a reader's first and > second read of the counter+bitmap. > 2)a writer increments the counter between a reader's first and second > read of counter+bitmap. > I.e. the read operation needs to be retried when some other thread has > made progress (with a write). > There is no spinning/waiting for other threads to complete. This makes > the design non-blocking (for readers). > > And it has almost no additional overhead because counter and bitmap > share one 32 bits variable. No additional load/store for reader and > writer. > IIUC, after this change if writer will start inserting the node, reader will not find any node with the same hash in cmap because it will check only "valid" slots. This breaks the cmap API and could lead to crash. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v3] northd: Add `status` management command
Allow the operator to query whether a ovn-northd process is
currently active for the standalone and clustered DB use case.
At present this information is only available in the log.
Signed-off-by: Frode Nordahl
---
northd/ovn-northd.8.xml | 9 -
northd/ovn-northd.c | 20 +++-
tests/ovn-northd.at | 9 +
3 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 78b1e84ad..d7833cda7 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -87,13 +87,20 @@
Returns "true" if ovn-northd is currently paused, "false" otherwise.
+
+ status
+
+Prints this server's status. Status will be "active" if ovn-northd has
+acquired OVSDB lock on NB DB, "standby" otherwise.
+
Active-Standby for High Availability
You may run ovn-northd more than once in an OVN deployment.
- OVN will automatically ensure that only one of them is active at a time.
+ When connected to a standalone or clustered DB setup, OVN will
+ automatically ensure that only one of them is active at a time.
If multiple instances of ovn-northd are running and the
active ovn-northd fails, one of the hot standby instances
of ovn-northd will automatically take over.
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 41e97f841..83ad6d518 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -55,6 +55,7 @@ static unixctl_cb_func ovn_northd_exit;
static unixctl_cb_func ovn_northd_pause;
static unixctl_cb_func ovn_northd_resume;
static unixctl_cb_func ovn_northd_is_paused;
+static unixctl_cb_func ovn_northd_status;
struct northd_context {
struct ovsdb_idl *ovnnb_idl;
@@ -10838,6 +10839,7 @@ main(int argc, char *argv[])
int retval;
bool exiting;
bool paused;
+bool had_lock;
fatal_ignore_sigpipe();
ovs_cmdl_proctitle_init(argc, argv);
@@ -10863,6 +10865,7 @@ main(int argc, char *argv[])
unixctl_command_register("resume", "", 0, 0, ovn_northd_resume, );
unixctl_command_register("is-paused", "", 0, 0, ovn_northd_is_paused,
);
+unixctl_command_register("status", "", 0, 0, ovn_northd_status, _lock);
daemonize_complete();
@@ -11068,11 +11071,11 @@ main(int argc, char *argv[])
* acquiring a lock called "ovn_northd" on the southbound database
* and then only performing DB transactions if the lock is held. */
ovsdb_idl_set_lock(ovnsb_idl_loop.idl, "ovn_northd");
-bool had_lock = false;
/* Main loop. */
exiting = false;
paused = false;
+had_lock = false;
while (!exiting) {
if (!paused) {
struct northd_context ctx = {
@@ -11180,3 +11183,18 @@ ovn_northd_is_paused(struct unixctl_conn *conn, int
argc OVS_UNUSED,
unixctl_command_reply(conn, "false");
}
}
+
+static void
+ovn_northd_status(struct unixctl_conn *conn, int argc OVS_UNUSED,
+ const char *argv[] OVS_UNUSED, void *had_lock_)
+{
+bool *had_lock = had_lock_;
+/*
+ * Use a labelled formatted output so we can add more to the status command
+ * later without breaking any consuming scripts
+ */
+struct ds s = DS_EMPTY_INITIALIZER;
+ds_put_format(, "Status: %s\n", *had_lock ? "active" : "standby");
+unixctl_command_reply(conn, ds_cstr());
+ds_destroy();
+}
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index da566f900..17e60b051 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -899,6 +899,15 @@ OVS_APP_EXIT_AND_WAIT([ovn-northd])
AT_CLEANUP
+AT_SETUP([ovn -- ovn-northd status])
+AT_SKIP_IF([test $HAVE_PYTHON = no])
+ovn_start
+
+AT_CHECK([as northd ovs-appctl -t ovn-northd status], [0], [Status: active
+])
+
+AT_CLEANUP
+
AT_SETUP([ovn -- ovn-northd pause and resume])
AT_SKIP_IF([test $HAVE_PYTHON = no])
ovn_start
--
2.24.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v5 ovn 4/4] ovn-controller: Fix use of dangling pointers in I-P runtime_data.
The incremental processing engine might stop a run before the
en_runtime_data node is processed. In such cases the ed_runtime_data
fields might contain pointers to already deleted SB records. For
example, if a port binding corresponding to a patch port is removed from
the SB database and the incremental processing engine aborts before the
en_runtime_data node is processed then the corresponding local_datapath
hashtable entry in ed_runtime_data is stale and will store a pointer to
the already freed sbrec_port_binding record.
This will cause invalid memory accesses in various places (e.g.,
pinctrl_run() -> prepare_ipv6_ras()).
To fix the issue we introduce the concept of "internal_data" vs "data" in
engine nodes. The first field, "internal_data", is data that can be accessed
by the incremental engine nodes handlers (data from other nodes must be
considered read-only and data from other nodes must not be accessed if the
nodes haven't been refreshed in the current iteration). The second field,
"data" is a pointer reset at engine_run() and if non-NULL indicates to
users outside the incremental engine that the data is safe to use.
This commit also adds an "is_valid()" method to engine nodes to allow
users to override the default behavior of determining if data is valid in a
node (e.g., for the ct-zones node the data is always safe to access).
CC: Han Zhou
Fixes: ca278d98a4f5 ("ovn-controller: Initial use of incremental engine - quiet
mode.")
Signed-off-by: Dumitru Ceara
---
controller/ovn-controller.c | 230 ++-
lib/inc-proc-eng.c | 25 -
lib/inc-proc-eng.h | 28 +
3 files changed, 163 insertions(+), 120 deletions(-)
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index d35862c..10792e5 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -741,8 +741,7 @@ struct ed_type_ofctrl_is_connected {
static void
en_ofctrl_is_connected_init(struct engine_node *node)
{
-struct ed_type_ofctrl_is_connected *data =
-(struct ed_type_ofctrl_is_connected *)node->data;
+struct ed_type_ofctrl_is_connected *data = node->internal_data;
data->connected = false;
}
@@ -754,8 +753,7 @@ en_ofctrl_is_connected_cleanup(struct engine_node *node
OVS_UNUSED)
static void
en_ofctrl_is_connected_run(struct engine_node *node)
{
-struct ed_type_ofctrl_is_connected *data =
-(struct ed_type_ofctrl_is_connected *)node->data;
+struct ed_type_ofctrl_is_connected *data = node->internal_data;
if (data->connected != ofctrl_is_connected()) {
data->connected = !data->connected;
engine_set_node_state(node, EN_UPDATED);
@@ -775,7 +773,7 @@ struct ed_type_addr_sets {
static void
en_addr_sets_init(struct engine_node *node)
{
-struct ed_type_addr_sets *as = (struct ed_type_addr_sets *)node->data;
+struct ed_type_addr_sets *as = node->internal_data;
shash_init(>addr_sets);
as->change_tracked = false;
sset_init(>new);
@@ -786,7 +784,7 @@ en_addr_sets_init(struct engine_node *node)
static void
en_addr_sets_cleanup(struct engine_node *node)
{
-struct ed_type_addr_sets *as = (struct ed_type_addr_sets *)node->data;
+struct ed_type_addr_sets *as = node->internal_data;
expr_const_sets_destroy(>addr_sets);
shash_destroy(>addr_sets);
sset_destroy(>new);
@@ -797,7 +795,7 @@ en_addr_sets_cleanup(struct engine_node *node)
static void
en_addr_sets_run(struct engine_node *node)
{
-struct ed_type_addr_sets *as = (struct ed_type_addr_sets *)node->data;
+struct ed_type_addr_sets *as = node->internal_data;
sset_clear(>new);
sset_clear(>deleted);
@@ -817,7 +815,7 @@ en_addr_sets_run(struct engine_node *node)
static bool
addr_sets_sb_address_set_handler(struct engine_node *node)
{
-struct ed_type_addr_sets *as = (struct ed_type_addr_sets *)node->data;
+struct ed_type_addr_sets *as = node->internal_data;
sset_clear(>new);
sset_clear(>deleted);
@@ -852,7 +850,7 @@ struct ed_type_port_groups{
static void
en_port_groups_init(struct engine_node *node)
{
-struct ed_type_port_groups *pg = (struct ed_type_port_groups *)node->data;
+struct ed_type_port_groups *pg = node->internal_data;
shash_init(>port_groups);
pg->change_tracked = false;
sset_init(>new);
@@ -863,7 +861,7 @@ en_port_groups_init(struct engine_node *node)
static void
en_port_groups_cleanup(struct engine_node *node)
{
-struct ed_type_port_groups *pg = (struct ed_type_port_groups *)node->data;
+struct ed_type_port_groups *pg = node->internal_data;
expr_const_sets_destroy(>port_groups);
shash_destroy(>port_groups);
sset_destroy(>new);
@@ -874,7 +872,7 @@ en_port_groups_cleanup(struct engine_node *node)
static void
en_port_groups_run(struct engine_node *node)
{
-struct ed_type_port_groups *pg = (struct ed_type_port_groups *)node->data;
+struct ed_type_port_groups *pg =
[ovs-dev] [PATCH v5 ovn 1/4] ovn-controller: Refactor I-P engine_run() tracking.
This commit simplifies the logic of calling engine_run and engine_need_run in
order to reduce the number of external variables required to track the result
of the last engine execution.
The engine code is also refactored a bit and the engine_run() function is
split in different functions that handle computing/recomputing a node.
Signed-off-by: Dumitru Ceara
---
controller/ovn-controller.c | 33 ++-
lib/inc-proc-eng.c | 124 +--
lib/inc-proc-eng.h |7 ++
3 files changed, 107 insertions(+), 57 deletions(-)
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 27cb488..c56190f 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -1942,7 +1942,6 @@ main(int argc, char *argv[])
_pkt);
uint64_t engine_run_id = 0;
-uint64_t old_engine_run_id = 0;
bool engine_run_done = true;
unsigned int ovs_cond_seqno = UINT_MAX;
@@ -1952,10 +1951,11 @@ main(int argc, char *argv[])
exiting = false;
restart = false;
while (!exiting) {
+engine_run_id++;
+
update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl);
update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl));
ofctrl_set_probe_interval(get_ofctrl_probe_interval(ovs_idl_loop.idl));
-old_engine_run_id = engine_run_id;
struct ovsdb_idl_txn *ovs_idl_txn = ovsdb_idl_loop_run(_idl_loop);
unsigned int new_ovs_cond_seqno
@@ -2047,12 +2047,12 @@ main(int argc, char *argv[])
if (engine_run_done) {
engine_set_abort_recompute(true);
engine_run_done = engine_run(_flow_output,
- ++engine_run_id);
+ engine_run_id);
}
} else {
engine_set_abort_recompute(false);
engine_run_done = true;
-engine_run(_flow_output, ++engine_run_id);
+engine_run(_flow_output, engine_run_id);
}
}
stopwatch_stop(CONTROLLER_LOOP_STOPWATCH_NAME,
@@ -2097,17 +2097,20 @@ main(int argc, char *argv[])
}
}
-if (old_engine_run_id == engine_run_id || !engine_run_done) {
-if (!engine_run_done || engine_need_run(_flow_output)) {
-VLOG_DBG("engine did not run, force recompute next time: "
- "br_int %p, chassis %p", br_int, chassis);
-engine_set_force_recompute(true);
-poll_immediate_wake();
-} else {
-VLOG_DBG("engine did not run, and it was not needed"
- " either: br_int %p, chassis %p",
- br_int, chassis);
-}
+if (engine_need_run(_flow_output, engine_run_id)) {
+VLOG_DBG("engine did not run, force recompute next time: "
+"br_int %p, chassis %p", br_int, chassis);
+engine_set_force_recompute(true);
+poll_immediate_wake();
+} else if (!engine_run_done) {
+VLOG_DBG("engine was aborted, force recompute next time: "
+ "br_int %p, chassis %p", br_int, chassis);
+engine_set_force_recompute(true);
+poll_immediate_wake();
+} else if (!engine_has_run(_flow_output, engine_run_id)) {
+VLOG_DBG("engine did not run, and it was not needed"
+ " either: br_int %p, chassis %p",
+ br_int, chassis);
} else {
engine_set_force_recompute(false);
}
diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c
index 1064a08..8a085e2 100644
--- a/lib/inc-proc-eng.c
+++ b/lib/inc-proc-eng.c
@@ -129,14 +129,72 @@ engine_ovsdb_node_add_index(struct engine_node *node,
const char *name,
}
bool
-engine_run(struct engine_node *node, uint64_t run_id)
+engine_has_run(struct engine_node *node, uint64_t run_id)
+{
+return node->run_id == run_id;
+}
+
+/* Do a full recompute (or at least try). If we're not allowed then
+ * mark the node as "aborted".
+ */
+static bool
+engine_recompute(struct engine_node *node, bool forced, bool allowed)
+{
+VLOG_DBG("node: %s, recompute (%s)", node->name,
+ forced ? "forced" : "triggered");
+
+if (!allowed) {
+VLOG_DBG("node: %s, recompute aborted", node->name);
+return false;
+}
+
+node->run(node);
+VLOG_DBG("node: %s, changed: %d", node->name, node->changed);
+return true;
+}
+
+/* Return true if the node could be computed without triggerring a
[ovs-dev] [PATCH v5 ovn 3/4] ovn-controller: Add separate I-P engine node for processing ct-zones.
Signed-off-by: Dumitru Ceara
---
controller/ovn-controller.c | 117 +--
1 file changed, 78 insertions(+), 39 deletions(-)
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 033eff4..d35862c 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -933,11 +933,6 @@ struct ed_type_runtime_data {
* _ */
struct sset local_lport_ids;
struct sset active_tunnels;
-
-/* connection tracking zones. */
-unsigned long ct_zone_bitmap[BITMAP_N_LONGS(MAX_CT_ZONES)];
-struct shash pending_ct_zones;
-struct simap ct_zones;
};
static void
@@ -945,24 +940,11 @@ en_runtime_data_init(struct engine_node *node)
{
struct ed_type_runtime_data *data =
(struct ed_type_runtime_data *)node->data;
-struct ovsrec_open_vswitch_table *ovs_table =
-(struct ovsrec_open_vswitch_table *)EN_OVSDB_GET(
-engine_get_input("OVS_open_vswitch", node));
-struct ovsrec_bridge_table *bridge_table =
-(struct ovsrec_bridge_table *)EN_OVSDB_GET(
-engine_get_input("OVS_bridge", node));
+
hmap_init(>local_datapaths);
sset_init(>local_lports);
sset_init(>local_lport_ids);
sset_init(>active_tunnels);
-shash_init(>pending_ct_zones);
-simap_init(>ct_zones);
-
-/* Initialize connection tracking zones. */
-memset(data->ct_zone_bitmap, 0, sizeof data->ct_zone_bitmap);
-bitmap_set1(data->ct_zone_bitmap, 0); /* Zone 0 is reserved. */
-restore_ct_zones(bridge_table, ovs_table,
- >ct_zones, data->ct_zone_bitmap);
}
static void
@@ -983,9 +965,6 @@ en_runtime_data_cleanup(struct engine_node *node)
free(cur_node);
}
hmap_destroy(>local_datapaths);
-
-simap_destroy(>ct_zones);
-shash_destroy(>pending_ct_zones);
}
static void
@@ -997,9 +976,6 @@ en_runtime_data_run(struct engine_node *node)
struct sset *local_lports = >local_lports;
struct sset *local_lport_ids = >local_lport_ids;
struct sset *active_tunnels = >active_tunnels;
-unsigned long *ct_zone_bitmap = data->ct_zone_bitmap;
-struct shash *pending_ct_zones = >pending_ct_zones;
-struct simap *ct_zones = >ct_zones;
static bool first_run = true;
if (first_run) {
@@ -1094,9 +1070,6 @@ en_runtime_data_run(struct engine_node *node)
ovs_table, local_datapaths,
local_lports, local_lport_ids);
-update_ct_zones(local_lports, local_datapaths, ct_zones,
-ct_zone_bitmap, pending_ct_zones);
-
engine_set_node_state(node, EN_UPDATED);
}
@@ -1138,6 +,55 @@ runtime_data_sb_port_binding_handler(struct engine_node
*node)
return !changed;
}
+/* Connection tracking zones. */
+struct ed_type_ct_zones {
+unsigned long bitmap[BITMAP_N_LONGS(MAX_CT_ZONES)];
+struct shash pending;
+struct simap current;
+};
+
+static void
+en_ct_zones_init(struct engine_node *node)
+{
+struct ed_type_ct_zones *data = node->data;
+struct ovsrec_open_vswitch_table *ovs_table =
+(struct ovsrec_open_vswitch_table *)EN_OVSDB_GET(
+engine_get_input("OVS_open_vswitch", node));
+struct ovsrec_bridge_table *bridge_table =
+(struct ovsrec_bridge_table *)EN_OVSDB_GET(
+engine_get_input("OVS_bridge", node));
+
+shash_init(>pending);
+simap_init(>current);
+
+memset(data->bitmap, 0, sizeof data->bitmap);
+bitmap_set1(data->bitmap, 0); /* Zone 0 is reserved. */
+restore_ct_zones(bridge_table, ovs_table, >current, data->bitmap);
+}
+
+static void
+en_ct_zones_cleanup(struct engine_node *node)
+{
+struct ed_type_ct_zones *data = node->data;
+
+simap_destroy(>current);
+shash_destroy(>pending);
+}
+
+static void
+en_ct_zones_run(struct engine_node *node)
+{
+struct ed_type_ct_zones *data = node->data;
+struct ed_type_runtime_data *rt_data =
+(struct ed_type_runtime_data *)engine_get_input(
+"runtime_data", node)->data;
+
+update_ct_zones(_data->local_lports, _data->local_datapaths,
+>current, data->bitmap, >pending);
+
+engine_set_node_state(node, EN_UPDATED);
+}
+
struct ed_type_mff_ovn_geneve {
enum mf_field_id mff_ovn_geneve;
};
@@ -1215,7 +1237,11 @@ en_flow_output_run(struct engine_node *node)
struct sset *local_lports = _data->local_lports;
struct sset *local_lport_ids = _data->local_lport_ids;
struct sset *active_tunnels = _data->active_tunnels;
-struct simap *ct_zones = _data->ct_zones;
+
+struct ed_type_ct_zones *ct_zones_data =
+(struct ed_type_ct_zones *)engine_get_input(
+"ct_zones", node)->data;
+struct simap *ct_zones = _zones_data->current;
struct ed_type_mff_ovn_geneve *ed_mff_ovn_geneve =
(struct ed_type_mff_ovn_geneve *)engine_get_input(
@@ -1445,7 +1471,11 @@ flow_output_sb_port_binding_handler(struct engine_node
*node)
[ovs-dev] [PATCH v5 ovn 2/4] ovn-controller: Add per node states to I-P engine.
This commit transforms the 'changed' field in struct engine_node in a
'state' field. Possible node states are:
- "Stale": data in the node is not up to date with the DB.
- "Updated": data in the node is valid but was updated during
the last run of the engine.
- "Valid": data in the node is valid and didn't change during
the last run of the engine.
- "Aborted": during the last run, processing was aborted for
this node.
This commit also further refactors the I-P engine:
- instead of recursively performing all the engine processing a
preprocessing stage is added (engine_get_nodes()) before the main processing
loop is executed in order to topologically sort nodes in the engine such
that all inputs of a given node appear in the sorted array before the node
itself. This simplifies a bit the code in engine_run().
- remove the need for using an engine_run_id by using the newly added states.
Signed-off-by: Dumitru Ceara
---
controller/ovn-controller.c | 88 ++---
lib/inc-proc-eng.c | 219 ---
lib/inc-proc-eng.h | 75 +++
3 files changed, 271 insertions(+), 111 deletions(-)
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index c56190f..033eff4 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -758,10 +758,10 @@ en_ofctrl_is_connected_run(struct engine_node *node)
(struct ed_type_ofctrl_is_connected *)node->data;
if (data->connected != ofctrl_is_connected()) {
data->connected = !data->connected;
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
return;
}
-node->changed = false;
+engine_set_node_state(node, EN_VALID);
}
struct ed_type_addr_sets {
@@ -811,7 +811,7 @@ en_addr_sets_run(struct engine_node *node)
addr_sets_init(as_table, >addr_sets);
as->change_tracked = false;
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
}
static bool
@@ -830,11 +830,14 @@ addr_sets_sb_address_set_handler(struct engine_node *node)
addr_sets_update(as_table, >addr_sets, >new,
>deleted, >updated);
-node->changed = !sset_is_empty(>new) || !sset_is_empty(>deleted)
-|| !sset_is_empty(>updated);
+if (!sset_is_empty(>new) || !sset_is_empty(>deleted) ||
+!sset_is_empty(>updated)) {
+engine_set_node_state(node, EN_UPDATED);
+} else {
+engine_set_node_state(node, EN_VALID);
+}
as->change_tracked = true;
-node->changed = true;
return true;
}
@@ -885,7 +888,7 @@ en_port_groups_run(struct engine_node *node)
port_groups_init(pg_table, >port_groups);
pg->change_tracked = false;
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
}
static bool
@@ -904,11 +907,14 @@ port_groups_sb_port_group_handler(struct engine_node
*node)
port_groups_update(pg_table, >port_groups, >new,
>deleted, >updated);
-node->changed = !sset_is_empty(>new) || !sset_is_empty(>deleted)
-|| !sset_is_empty(>updated);
+if (!sset_is_empty(>new) || !sset_is_empty(>deleted) ||
+!sset_is_empty(>updated)) {
+engine_set_node_state(node, EN_UPDATED);
+} else {
+engine_set_node_state(node, EN_VALID);
+}
pg->change_tracked = true;
-node->changed = true;
return true;
}
@@ -1091,7 +1097,7 @@ en_runtime_data_run(struct engine_node *node)
update_ct_zones(local_lports, local_datapaths, ct_zones,
ct_zone_bitmap, pending_ct_zones);
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
}
static bool
@@ -1157,10 +1163,10 @@ en_mff_ovn_geneve_run(struct engine_node *node)
enum mf_field_id mff_ovn_geneve = ofctrl_get_mf_field_id();
if (data->mff_ovn_geneve != mff_ovn_geneve) {
data->mff_ovn_geneve = mff_ovn_geneve;
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
return;
}
-node->changed = false;
+engine_set_node_state(node, EN_VALID);
}
struct ed_type_flow_output {
@@ -1322,7 +1328,7 @@ en_flow_output_run(struct engine_node *node)
active_tunnels,
flow_table);
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
}
static bool
@@ -1404,7 +1410,7 @@ flow_output_sb_logical_flow_handler(struct engine_node
*node)
flow_table, group_table, meter_table, lfrr,
conj_id_ofs);
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
return handled;
}
@@ -1427,7 +1433,7 @@ flow_output_sb_mac_binding_handler(struct engine_node
*node)
lflow_handle_changed_neighbors(sbrec_port_binding_by_name,
mac_binding_table, flow_table);
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
return true;
}
@@
[ovs-dev] [PATCH v5 ovn 0/4] Refactor I-P engine and fix use after free.
The incremental processing engine might stop a run before the
en_runtime_data node is processed. In such cases the ed_runtime_data
fields might contain pointers to already deleted SB records. For
example, if a port binding corresponding to a patch port is removed from
the SB database and the incremental processing engine aborts before the
en_runtime_data node is processed then the corresponding local_datapath
hashtable entry in ed_runtime_data is stale and will store a pointer to
the already freed sbrec_port_binding record.
This will cause invalid memory accesses in various places (e.g.,
pinctrl_run() -> prepare_ipv6_ras()).
This series fixes the issue (patch4) but to make the fix generic and
easier to debug it first refactors the incremental processing engine in
the following way:
- patch1: split engine_run() in smaller functional parts and simplify the
logic of calling engine_run and engine_need_run in the main loop.
- patch2: remove recursion from the I-P engine code. Introduce node states
to track validity of node data.
- patch3: move ct-zones to its own engine node in order to remove dependencies
on other runtime data.
CC: Han Zhou
Fixes: ca278d98a4f5 ("ovn-controller: Initial use of incremental engine - quiet
mode.")
Signed-off-by: Dumitru Ceara
Dumitru Ceara (4):
ovn-controller: Refactor I-P engine_run() tracking.
ovn-controller: Add per node states to I-P engine.
ovn-controller: Add separate I-P engine node for processing ct-zones.
ovn-controller: Fix use of dangling pointers in I-P runtime_data.
controller/ovn-controller.c | 418 ---
lib/inc-proc-eng.c | 314 +---
lib/inc-proc-eng.h | 104 +--
3 files changed, 564 insertions(+), 272 deletions(-)
---
v5:
- Rebase.
v4:
- Address Numan's comments:
- Fix engine_need_run().
v3:
- split the change in series.
- Address Han's comments:
- fix the data encapsulation issue.
- add is_valid method to nodes.
- add internal_data/data fields to nodes as it makes it easier to write
the code instead of adding an "engine_get_data()" API.
v2: Address Han's comments:
- call engine_node_valid() in all the places where node local data is
used.
- move out "global" data outside the engine nodes. Make a clear
separation between data that can be safely used at any time and data
that can be used only when the engine run was successful.
- add a debug log for iterations when the engine didn't run.
- refactor a bit more the incremental engine code.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] Investment Proposal
Dear Sir, My name is Samuel Okono, a Senior Investment Consultant here in Ghana West Africa. I have a client in Ghana that is interested to Invest $108 million USD in your country and would like to engage you or your company to manage the investment on this project. Please my Client needs a real Professional with Business Experience to manage the said Funds on Profit Percentage basis. If you are interested and know that you have the Ability to manage the said Funds, kindly respond to me via my private Email: samuelokon...@gmail.com with your direct telephone number for full discussion of this offer. Please if you are not sure that you can manage such amount of Money, just let me know to enable me source for another capable business man who can manage the said Funds. Thank you. Respectfully, Samuel Okono ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 ovn 4/4] ovn-controller: Fix use of dangling pointers in I-P runtime_data.
Bleep bloop. Greetings Dumitru Ceara, 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. git-am: fatal: sha1 information is lacking or useless (controller/ovn-controller.c). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 ovn-controller: Fix use of dangling pointers in I-P runtime_data. The copy of the patch that failed is found in: /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/OVN/.git/rebase-apply/patch When you have resolved this problem, run "git am --resolved". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". 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 v4 ovn 3/4] ovn-controller: Add separate I-P engine node for processing ct-zones.
Signed-off-by: Dumitru Ceara
---
controller/ovn-controller.c | 117 +--
1 file changed, 78 insertions(+), 39 deletions(-)
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index a088fb0..465f831 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -933,11 +933,6 @@ struct ed_type_runtime_data {
* _ */
struct sset local_lport_ids;
struct sset active_tunnels;
-
-/* connection tracking zones. */
-unsigned long ct_zone_bitmap[BITMAP_N_LONGS(MAX_CT_ZONES)];
-struct shash pending_ct_zones;
-struct simap ct_zones;
};
static void
@@ -945,24 +940,11 @@ en_runtime_data_init(struct engine_node *node)
{
struct ed_type_runtime_data *data =
(struct ed_type_runtime_data *)node->data;
-struct ovsrec_open_vswitch_table *ovs_table =
-(struct ovsrec_open_vswitch_table *)EN_OVSDB_GET(
-engine_get_input("OVS_open_vswitch", node));
-struct ovsrec_bridge_table *bridge_table =
-(struct ovsrec_bridge_table *)EN_OVSDB_GET(
-engine_get_input("OVS_bridge", node));
+
hmap_init(>local_datapaths);
sset_init(>local_lports);
sset_init(>local_lport_ids);
sset_init(>active_tunnels);
-shash_init(>pending_ct_zones);
-simap_init(>ct_zones);
-
-/* Initialize connection tracking zones. */
-memset(data->ct_zone_bitmap, 0, sizeof data->ct_zone_bitmap);
-bitmap_set1(data->ct_zone_bitmap, 0); /* Zone 0 is reserved. */
-restore_ct_zones(bridge_table, ovs_table,
- >ct_zones, data->ct_zone_bitmap);
}
static void
@@ -983,9 +965,6 @@ en_runtime_data_cleanup(struct engine_node *node)
free(cur_node);
}
hmap_destroy(>local_datapaths);
-
-simap_destroy(>ct_zones);
-shash_destroy(>pending_ct_zones);
}
static void
@@ -997,9 +976,6 @@ en_runtime_data_run(struct engine_node *node)
struct sset *local_lports = >local_lports;
struct sset *local_lport_ids = >local_lport_ids;
struct sset *active_tunnels = >active_tunnels;
-unsigned long *ct_zone_bitmap = data->ct_zone_bitmap;
-struct shash *pending_ct_zones = >pending_ct_zones;
-struct simap *ct_zones = >ct_zones;
static bool first_run = true;
if (first_run) {
@@ -1094,9 +1070,6 @@ en_runtime_data_run(struct engine_node *node)
ovs_table, local_datapaths,
local_lports, local_lport_ids);
-update_ct_zones(local_lports, local_datapaths, ct_zones,
-ct_zone_bitmap, pending_ct_zones);
-
engine_set_node_state(node, EN_UPDATED);
}
@@ -1138,6 +,55 @@ runtime_data_sb_port_binding_handler(struct engine_node
*node)
return !changed;
}
+/* Connection tracking zones. */
+struct ed_type_ct_zones {
+unsigned long bitmap[BITMAP_N_LONGS(MAX_CT_ZONES)];
+struct shash pending;
+struct simap current;
+};
+
+static void
+en_ct_zones_init(struct engine_node *node)
+{
+struct ed_type_ct_zones *data = node->data;
+struct ovsrec_open_vswitch_table *ovs_table =
+(struct ovsrec_open_vswitch_table *)EN_OVSDB_GET(
+engine_get_input("OVS_open_vswitch", node));
+struct ovsrec_bridge_table *bridge_table =
+(struct ovsrec_bridge_table *)EN_OVSDB_GET(
+engine_get_input("OVS_bridge", node));
+
+shash_init(>pending);
+simap_init(>current);
+
+memset(data->bitmap, 0, sizeof data->bitmap);
+bitmap_set1(data->bitmap, 0); /* Zone 0 is reserved. */
+restore_ct_zones(bridge_table, ovs_table, >current, data->bitmap);
+}
+
+static void
+en_ct_zones_cleanup(struct engine_node *node)
+{
+struct ed_type_ct_zones *data = node->data;
+
+simap_destroy(>current);
+shash_destroy(>pending);
+}
+
+static void
+en_ct_zones_run(struct engine_node *node)
+{
+struct ed_type_ct_zones *data = node->data;
+struct ed_type_runtime_data *rt_data =
+(struct ed_type_runtime_data *)engine_get_input(
+"runtime_data", node)->data;
+
+update_ct_zones(_data->local_lports, _data->local_datapaths,
+>current, data->bitmap, >pending);
+
+engine_set_node_state(node, EN_UPDATED);
+}
+
struct ed_type_mff_ovn_geneve {
enum mf_field_id mff_ovn_geneve;
};
@@ -1215,7 +1237,11 @@ en_flow_output_run(struct engine_node *node)
struct sset *local_lports = _data->local_lports;
struct sset *local_lport_ids = _data->local_lport_ids;
struct sset *active_tunnels = _data->active_tunnels;
-struct simap *ct_zones = _data->ct_zones;
+
+struct ed_type_ct_zones *ct_zones_data =
+(struct ed_type_ct_zones *)engine_get_input(
+"ct_zones", node)->data;
+struct simap *ct_zones = _zones_data->current;
struct ed_type_mff_ovn_geneve *ed_mff_ovn_geneve =
(struct ed_type_mff_ovn_geneve *)engine_get_input(
@@ -1445,7 +1471,11 @@ flow_output_sb_port_binding_handler(struct engine_node
*node)
[ovs-dev] [PATCH v4 ovn 4/4] ovn-controller: Fix use of dangling pointers in I-P runtime_data.
The incremental processing engine might stop a run before the
en_runtime_data node is processed. In such cases the ed_runtime_data
fields might contain pointers to already deleted SB records. For
example, if a port binding corresponding to a patch port is removed from
the SB database and the incremental processing engine aborts before the
en_runtime_data node is processed then the corresponding local_datapath
hashtable entry in ed_runtime_data is stale and will store a pointer to
the already freed sbrec_port_binding record.
This will cause invalid memory accesses in various places (e.g.,
pinctrl_run() -> prepare_ipv6_ras()).
To fix the issue we introduce the concept of "internal_data" vs "data" in
engine nodes. The first field, "internal_data", is data that can be accessed
by the incremental engine nodes handlers (data from other nodes must be
considered read-only and data from other nodes must not be accessed if the
nodes haven't been refreshed in the current iteration). The second field,
"data" is a pointer reset at engine_run() and if non-NULL indicates to
users outside the incremental engine that the data is safe to use.
This commit also adds an "is_valid()" method to engine nodes to allow
users to override the default behavior of determining if data is valid in a
node (e.g., for the ct-zones node the data is always safe to access).
CC: Han Zhou
Fixes: ca278d98a4f5 ("ovn-controller: Initial use of incremental engine - quiet
mode.")
Signed-off-by: Dumitru Ceara
---
controller/ovn-controller.c | 227 ++-
lib/inc-proc-eng.c | 25 -
lib/inc-proc-eng.h | 28 +
3 files changed, 162 insertions(+), 118 deletions(-)
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 465f831..458f9a0 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -741,8 +741,7 @@ struct ed_type_ofctrl_is_connected {
static void
en_ofctrl_is_connected_init(struct engine_node *node)
{
-struct ed_type_ofctrl_is_connected *data =
-(struct ed_type_ofctrl_is_connected *)node->data;
+struct ed_type_ofctrl_is_connected *data = node->internal_data;
data->connected = false;
}
@@ -754,8 +753,7 @@ en_ofctrl_is_connected_cleanup(struct engine_node *node
OVS_UNUSED)
static void
en_ofctrl_is_connected_run(struct engine_node *node)
{
-struct ed_type_ofctrl_is_connected *data =
-(struct ed_type_ofctrl_is_connected *)node->data;
+struct ed_type_ofctrl_is_connected *data = node->internal_data;
if (data->connected != ofctrl_is_connected()) {
data->connected = !data->connected;
engine_set_node_state(node, EN_UPDATED);
@@ -775,7 +773,7 @@ struct ed_type_addr_sets {
static void
en_addr_sets_init(struct engine_node *node)
{
-struct ed_type_addr_sets *as = (struct ed_type_addr_sets *)node->data;
+struct ed_type_addr_sets *as = node->internal_data;
shash_init(>addr_sets);
as->change_tracked = false;
sset_init(>new);
@@ -786,7 +784,7 @@ en_addr_sets_init(struct engine_node *node)
static void
en_addr_sets_cleanup(struct engine_node *node)
{
-struct ed_type_addr_sets *as = (struct ed_type_addr_sets *)node->data;
+struct ed_type_addr_sets *as = node->internal_data;
expr_const_sets_destroy(>addr_sets);
shash_destroy(>addr_sets);
sset_destroy(>new);
@@ -797,7 +795,7 @@ en_addr_sets_cleanup(struct engine_node *node)
static void
en_addr_sets_run(struct engine_node *node)
{
-struct ed_type_addr_sets *as = (struct ed_type_addr_sets *)node->data;
+struct ed_type_addr_sets *as = node->internal_data;
sset_clear(>new);
sset_clear(>deleted);
@@ -817,7 +815,7 @@ en_addr_sets_run(struct engine_node *node)
static bool
addr_sets_sb_address_set_handler(struct engine_node *node)
{
-struct ed_type_addr_sets *as = (struct ed_type_addr_sets *)node->data;
+struct ed_type_addr_sets *as = node->internal_data;
sset_clear(>new);
sset_clear(>deleted);
@@ -852,7 +850,7 @@ struct ed_type_port_groups{
static void
en_port_groups_init(struct engine_node *node)
{
-struct ed_type_port_groups *pg = (struct ed_type_port_groups *)node->data;
+struct ed_type_port_groups *pg = node->internal_data;
shash_init(>port_groups);
pg->change_tracked = false;
sset_init(>new);
@@ -863,7 +861,7 @@ en_port_groups_init(struct engine_node *node)
static void
en_port_groups_cleanup(struct engine_node *node)
{
-struct ed_type_port_groups *pg = (struct ed_type_port_groups *)node->data;
+struct ed_type_port_groups *pg = node->internal_data;
expr_const_sets_destroy(>port_groups);
shash_destroy(>port_groups);
sset_destroy(>new);
@@ -874,7 +872,7 @@ en_port_groups_cleanup(struct engine_node *node)
static void
en_port_groups_run(struct engine_node *node)
{
-struct ed_type_port_groups *pg = (struct ed_type_port_groups *)node->data;
+struct ed_type_port_groups *pg =
[ovs-dev] [PATCH v4 ovn 2/4] ovn-controller: Add per node states to I-P engine.
This commit transforms the 'changed' field in struct engine_node in a
'state' field. Possible node states are:
- "Stale": data in the node is not up to date with the DB.
- "Updated": data in the node is valid but was updated during
the last run of the engine.
- "Valid": data in the node is valid and didn't change during
the last run of the engine.
- "Aborted": during the last run, processing was aborted for
this node.
This commit also further refactors the I-P engine:
- instead of recursively performing all the engine processing a
preprocessing stage is added (engine_get_nodes()) before the main processing
loop is executed in order to topologically sort nodes in the engine such
that all inputs of a given node appear in the sorted array before the node
itself. This simplifies a bit the code in engine_run().
- remove the need for using an engine_run_id by using the newly added states.
Signed-off-by: Dumitru Ceara
---
v2:
- Address Numan's comment:
- Fix engine_need_run().
---
controller/ovn-controller.c | 88 ++---
lib/inc-proc-eng.c | 219 ---
lib/inc-proc-eng.h | 75 +++
3 files changed, 271 insertions(+), 111 deletions(-)
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 3922f3d..a088fb0 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -758,10 +758,10 @@ en_ofctrl_is_connected_run(struct engine_node *node)
(struct ed_type_ofctrl_is_connected *)node->data;
if (data->connected != ofctrl_is_connected()) {
data->connected = !data->connected;
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
return;
}
-node->changed = false;
+engine_set_node_state(node, EN_VALID);
}
struct ed_type_addr_sets {
@@ -811,7 +811,7 @@ en_addr_sets_run(struct engine_node *node)
addr_sets_init(as_table, >addr_sets);
as->change_tracked = false;
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
}
static bool
@@ -830,11 +830,14 @@ addr_sets_sb_address_set_handler(struct engine_node *node)
addr_sets_update(as_table, >addr_sets, >new,
>deleted, >updated);
-node->changed = !sset_is_empty(>new) || !sset_is_empty(>deleted)
-|| !sset_is_empty(>updated);
+if (!sset_is_empty(>new) || !sset_is_empty(>deleted) ||
+!sset_is_empty(>updated)) {
+engine_set_node_state(node, EN_UPDATED);
+} else {
+engine_set_node_state(node, EN_VALID);
+}
as->change_tracked = true;
-node->changed = true;
return true;
}
@@ -885,7 +888,7 @@ en_port_groups_run(struct engine_node *node)
port_groups_init(pg_table, >port_groups);
pg->change_tracked = false;
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
}
static bool
@@ -904,11 +907,14 @@ port_groups_sb_port_group_handler(struct engine_node
*node)
port_groups_update(pg_table, >port_groups, >new,
>deleted, >updated);
-node->changed = !sset_is_empty(>new) || !sset_is_empty(>deleted)
-|| !sset_is_empty(>updated);
+if (!sset_is_empty(>new) || !sset_is_empty(>deleted) ||
+!sset_is_empty(>updated)) {
+engine_set_node_state(node, EN_UPDATED);
+} else {
+engine_set_node_state(node, EN_VALID);
+}
pg->change_tracked = true;
-node->changed = true;
return true;
}
@@ -1091,7 +1097,7 @@ en_runtime_data_run(struct engine_node *node)
update_ct_zones(local_lports, local_datapaths, ct_zones,
ct_zone_bitmap, pending_ct_zones);
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
}
static bool
@@ -1157,10 +1163,10 @@ en_mff_ovn_geneve_run(struct engine_node *node)
enum mf_field_id mff_ovn_geneve = ofctrl_get_mf_field_id();
if (data->mff_ovn_geneve != mff_ovn_geneve) {
data->mff_ovn_geneve = mff_ovn_geneve;
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
return;
}
-node->changed = false;
+engine_set_node_state(node, EN_VALID);
}
struct ed_type_flow_output {
@@ -1322,7 +1328,7 @@ en_flow_output_run(struct engine_node *node)
active_tunnels,
flow_table);
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
}
static bool
@@ -1404,7 +1410,7 @@ flow_output_sb_logical_flow_handler(struct engine_node
*node)
flow_table, group_table, meter_table, lfrr,
conj_id_ofs);
-node->changed = true;
+engine_set_node_state(node, EN_UPDATED);
return handled;
}
@@ -1427,7 +1433,7 @@ flow_output_sb_mac_binding_handler(struct engine_node
*node)
lflow_handle_changed_neighbors(sbrec_port_binding_by_name,
mac_binding_table, flow_table);
-node->changed = true;
+
[ovs-dev] [PATCH v4 ovn 1/4] ovn-controller: Refactor I-P engine_run() tracking.
This commit simplifies the logic of calling engine_run and engine_need_run in
order to reduce the number of external variables required to track the result
of the last engine execution.
The engine code is also refactored a bit and the engine_run() function is
split in different functions that handle computing/recomputing a node.
Signed-off-by: Dumitru Ceara
---
controller/ovn-controller.c | 33 ++-
lib/inc-proc-eng.c | 124 +--
lib/inc-proc-eng.h |7 ++
3 files changed, 107 insertions(+), 57 deletions(-)
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 9ab98be..3922f3d 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -1942,7 +1942,6 @@ main(int argc, char *argv[])
_pkt);
uint64_t engine_run_id = 0;
-uint64_t old_engine_run_id = 0;
bool engine_run_done = true;
unsigned int ovs_cond_seqno = UINT_MAX;
@@ -1952,10 +1951,11 @@ main(int argc, char *argv[])
exiting = false;
restart = false;
while (!exiting) {
+engine_run_id++;
+
update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl);
update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl));
ofctrl_set_probe_interval(get_ofctrl_probe_interval(ovs_idl_loop.idl));
-old_engine_run_id = engine_run_id;
struct ovsdb_idl_txn *ovs_idl_txn = ovsdb_idl_loop_run(_idl_loop);
unsigned int new_ovs_cond_seqno
@@ -2047,12 +2047,12 @@ main(int argc, char *argv[])
if (engine_run_done) {
engine_set_abort_recompute(true);
engine_run_done = engine_run(_flow_output,
- ++engine_run_id);
+ engine_run_id);
}
} else {
engine_set_abort_recompute(false);
engine_run_done = true;
-engine_run(_flow_output, ++engine_run_id);
+engine_run(_flow_output, engine_run_id);
}
}
stopwatch_stop(CONTROLLER_LOOP_STOPWATCH_NAME,
@@ -2095,17 +2095,20 @@ main(int argc, char *argv[])
}
}
-if (old_engine_run_id == engine_run_id || !engine_run_done) {
-if (!engine_run_done || engine_need_run(_flow_output)) {
-VLOG_DBG("engine did not run, force recompute next time: "
- "br_int %p, chassis %p", br_int, chassis);
-engine_set_force_recompute(true);
-poll_immediate_wake();
-} else {
-VLOG_DBG("engine did not run, and it was not needed"
- " either: br_int %p, chassis %p",
- br_int, chassis);
-}
+if (engine_need_run(_flow_output, engine_run_id)) {
+VLOG_DBG("engine did not run, force recompute next time: "
+"br_int %p, chassis %p", br_int, chassis);
+engine_set_force_recompute(true);
+poll_immediate_wake();
+} else if (!engine_run_done) {
+VLOG_DBG("engine was aborted, force recompute next time: "
+ "br_int %p, chassis %p", br_int, chassis);
+engine_set_force_recompute(true);
+poll_immediate_wake();
+} else if (!engine_has_run(_flow_output, engine_run_id)) {
+VLOG_DBG("engine did not run, and it was not needed"
+ " either: br_int %p, chassis %p",
+ br_int, chassis);
} else {
engine_set_force_recompute(false);
}
diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c
index 1064a08..8a085e2 100644
--- a/lib/inc-proc-eng.c
+++ b/lib/inc-proc-eng.c
@@ -129,14 +129,72 @@ engine_ovsdb_node_add_index(struct engine_node *node,
const char *name,
}
bool
-engine_run(struct engine_node *node, uint64_t run_id)
+engine_has_run(struct engine_node *node, uint64_t run_id)
+{
+return node->run_id == run_id;
+}
+
+/* Do a full recompute (or at least try). If we're not allowed then
+ * mark the node as "aborted".
+ */
+static bool
+engine_recompute(struct engine_node *node, bool forced, bool allowed)
+{
+VLOG_DBG("node: %s, recompute (%s)", node->name,
+ forced ? "forced" : "triggered");
+
+if (!allowed) {
+VLOG_DBG("node: %s, recompute aborted", node->name);
+return false;
+}
+
+node->run(node);
+VLOG_DBG("node: %s, changed: %d", node->name, node->changed);
+return true;
+}
+
+/* Return true if the node could be computed without triggerring a
Re: [ovs-dev] [PATCH ovn v2 0/3] Health check feature for Load balancer backends
On Tue, Nov 5, 2019 at 2:53 PM wrote: > > From: Numan Siddique > > This series adds load balancer health check feature. With this > ovn-controllers will periodically check the status of the backend > services. Only those services which are online/active will be considered > for load balancing. > > Right now this feature is restricted to IPv4 Load balancers only. > CMS needs to enable this feature and the load balancer vips and backends > should have L4 port defined. > > For TCP backends, the local ovn-controller which binds that service's > VIF, will periodically send a SYN packet and would expect SYN-ACK > response to set the status of that service to online. If no response > is received within the timeout, then the service status is set to > offline. > > For UDP backends, the local ovn-controller which binds that service's > VIF, will periodically send an UDP packet and expects no reply. If no > reply is received within the timeout vallue, the service status is set > to online. In case the service is down, then ovn-controller expects > ICMP unreachable packet and upon receiving this ICMP packets, it sets > the status to offline. > > ovn-northd adds only those backends whose status is 'online' or empty to > the ct_lb action. > > > v1 -> v2 > --- > * Addressed review comment from Mark in p1. > * Rebased to latest master and resolved conflicts. Thanks for the reviews Mark. I applied this series to master. Numan > > Numan Siddique (3): > ovn-northd: Add support for Load Balancer health check > Add a new action - handle_svc_check > Send service monitor health checks > > controller/ovn-controller.c | 2 + > controller/pinctrl.c | 775 -- > controller/pinctrl.h | 2 + > include/ovn/actions.h | 17 +- > lib/actions.c | 42 ++ > northd/ovn-northd.8.xml | 85 +++- > northd/ovn-northd.c | 510 -- > ovn-nb.ovsschema | 25 +- > ovn-nb.xml| 68 +++ > ovn-sb.ovsschema | 33 +- > ovn-sb.xml| 102 + > tests/ovn-northd.at | 215 ++ > tests/ovn.at | 132 ++ > tests/system-common-macros.at | 1 + > tests/system-ovn.at | 180 > utilities/ovn-trace.c | 3 + > 16 files changed, 2119 insertions(+), 73 deletions(-) > > -- > 2.23.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 v2] northd: Add `is-active` management command
On Mon, Nov 18, 2019 at 1:21 PM Frode Nordahl
wrote:
>
> On Mon, Nov 18, 2019 at 8:11 AM Numan Siddique wrote:
> >
> > On Sat, Nov 16, 2019 at 2:46 AM Frode Nordahl
> > wrote:
> > >
> > > When ovn-northd is connected to clustered OVN DB servers a OVSDB
> > > locking feature is used to ensure only one ovn-northd process is
> > > active at a time.
> > >
> > > This patch adds a `is-active` management command that allows the
> > > operator to query whether a ovn-northd process is currently active
> > > or not.
> > >
> > > At present this information is only available in the log.
> > >
> > > Signed-off-by: Frode Nordahl
> >
> > Thanks for the patch. This command would be useful.
> > HA for ovn-northd (active/standby) is supported using the lock
> > mechanism and it doesn't
> > matter if ovn-northd connects to the clustered db or standalone. This
> > patch assumes
> > that locking feature is used only for clustered deployment which is wrong.
>
> Thank you for the review Numan, you are right, the lock will be used
> for both scenarios.
>
> > Also we have commands - "is-paused", "pause" and "resume". "is-active"
> > command could confuse
> > the user.
>
> Yes, I thought a bit about that and it will indeed be a bit confusing
> as the commands are used for two different methods of northd HA.
>
> How would you feel about naming the command "has-lock" instead, which
> would accurately describe what the management command actually tests
> for?
Well lock mechanism is something internal to ovn-northd and ovsdb-server. So not
sure that is the right command name.
>
> > So can you please update the documentation properly ?
>
> Will do.
>
> > Thanks
> > Numan
> >
> >
> > > ---
> > > northd/ovn-northd.8.xml | 19 +++
> > > northd/ovn-northd.c | 18 +-
> > > 2 files changed, 36 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> > > index 344cc0dbf..e712000f3 100644
> > > --- a/northd/ovn-northd.8.xml
> > > +++ b/northd/ovn-northd.8.xml
> > > @@ -87,6 +87,12 @@
> > >
> > > Returns "true" if ovn-northd is currently paused, "false"
> > > otherwise.
> > >
> > > +
> > > + is-active
> > > +
> > > +When ovn-northd is connected to clustered OVN DB servers, this
> > > returns
> > > +"true" if ovn-northd is currently active, "false" otherwise.
> >
> > You could probably say something like -
> > "Returns true if ovn-northd has acquired OVSDB lock and is currently
> > active, false otherwise.
>
> Yes, that makes sense.
>
> > > +
> > >
> > >
> > >
> > > @@ -130,6 +136,19 @@
> > >DB changes.
> > >
> > >
> > > + Active-Standby with clustered OVN DB servers
> > > +
> > > + When ovn-northd is connected to clustered OVN DB
> > > servers a
> > > + OVSDB locking feature will be used to ensure only one
> > > + ovn-northd process is active at a time.
> > > +
> > > +
> > > +
> > > + The ovn-northd daemon will write an entry in its log
> > > when
> > > + it becomes active. You may query the active status at any time
> > > with
> > > + the is-active management command.
> > > +
> >
> > We probably don't need this section as the documentation here [1]
> > already mentions about it.
> >
> > [1] - https://github.com/ovn-org/ovn/blob/master/northd/ovn-northd.8.xml#L95
>
> When I read the existing section I get the impression that ovn-northd
> only supports active/passive with replicated OVSDB server and manual
> pause of non-active northd daemons. So the intention of the change
> was to make that a bit more clear.
>
> What do you think about a change like this in the first paragraph:
> ...
> When connected to clustered DB servers, OVN will automatically ensure that
> only
> one of them is active at a time.
Sounds fine to me.
The reason why the same lock mechanism works for both standalone and
clustered setup is
because all the ovn-northd's in the cluster setup will connect to the
leader of the cluster
and hence only one ovn-northd will get the lock and that becomes active.
Thanks
Numan
> ...
>
> --
> Frode Nordahl
>
> > Thanks
> > Numan
> >
> >
> > > +
> > > Logical Flow Table Structure
> > >
> > >
> > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > > index 6742bc002..31a744f4d 100644
> > > --- a/northd/ovn-northd.c
> > > +++ b/northd/ovn-northd.c
> > > @@ -55,6 +55,7 @@ static unixctl_cb_func ovn_northd_exit;
> > > static unixctl_cb_func ovn_northd_pause;
> > > static unixctl_cb_func ovn_northd_resume;
> > > static unixctl_cb_func ovn_northd_is_paused;
> > > +static unixctl_cb_func ovn_northd_is_active;
> > >
> > > struct northd_context {
> > > struct ovsdb_idl *ovnnb_idl;
> > > @@ -10425,6 +10426,7 @@ main(int argc, char *argv[])
> > > int retval;
> > > bool exiting;
> > > bool paused;
> > > +bool had_lock;
> > >
> > > fatal_ignore_sigpipe();
> > >
[ovs-dev] sans prérequis
Formations en Magntisme Thrapeutique Le Magntisme Thrapeutique permet par simple imposition des mains de soulager en r-harmonisant les nergies. Dcouvrez, comprenez et contrlez ces incroyables pouvoirs qui sont cachs en vous ! Cette formation Certifiante de Praticien en Magntisme Thrapeutique vise dans un premier temps, faire prendre conscience chaque participant de son potentiel nergtique pour, par la suite, par le biais d’exercices ludiques et de techniques simples, lui apprendre l’utiliser bon escient et l’amplifier. Le cursus de Technicien est OUVERT A TOUS. Le Magntisme est une thrapie efficace, sans danger, facile apprendre et pratiquer, afin de pouvoir aider les autres retrouver Sant et bien-tre tant physique que psychologique. Un magntiseur ne lutte pas contre les maladies, il ne cherche pas faire disparatre les symptmes, mais en aidant le consultant r-quilibrer ses nergies il lui permet de retrouver peu peu l’harmonie qui avait t perturbe par la maladie. Grce au travail de groupe chacun aura l’opportunit d’exprimenter tantt le rle de thrapeute et tantt celui de consultant . Nous limitons le nombre de participants chaque session, afin de pouvoir donner le maximum chacun. Cliquez ICI pour en savoir plus ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 ovn 2/4] ovn-controller: Add per node states to I-P engine.
On Mon, Nov 18, 2019 at 8:38 AM Numan Siddique wrote:
>
> Thanks for this series Dumitru. This is really helpful.
>
> Few comments below.
>
>
> On Thu, Nov 14, 2019 at 10:39 PM Dumitru Ceara wrote:
> >
> > This commit transforms the 'changed' field in struct engine_node in a
> > 'state' field. Possible node states are:
> > - "Stale": data in the node is not up to date with the DB.
> > - "Updated": data in the node is valid but was updated during
> > the last run of the engine.
> > - "Valid": data in the node is valid and didn't change during
> > the last run of the engine.
> > - "Aborted": during the last run, processing was aborted for
> > this node.
> >
> > This commit also further refactors the I-P engine:
> > - instead of recursively performing all the engine processing a
> > preprocessing stage is added (engine_get_nodes()) before the main
> > processing
> > loop is executed in order to topologically sort nodes in the engine such
> > that all inputs of a given node appear in the sorted array before the node
> > itself. This simplifies a bit the code in engine_run().
> > - remove the need for using an engine_run_id by using the newly added
> > states.
> >
> > Signed-off-by: Dumitru Ceara
> > ---
>
> After applying this patch I notice that adding a logical switch or
> logical router is resulting
> the engine_run() to be called twice i.e the function
> engine_need_run() always returns true when I
> run "ovn-nbctl ls-add sw0" and engine_run() is called again.
>
> If you enable debug and add a logical switch you will see [1] to be
> hit all the time which is not the case
> before this patch.
>
> Thanks
> Numan
Nice catch, sorry about that.
I forgot to add a check to see if the engine was run on the root node
(en_flow_output) in the iteration. I'll fix it in v4.
Thanks,
Dumitru
>
>
> > controller/ovn-controller.c | 88 ++---
> > lib/inc-proc-eng.c | 218
> > ---
> > lib/inc-proc-eng.h | 74 +++
> > 3 files changed, 267 insertions(+), 113 deletions(-)
> >
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 3922f3d..4f8ceae 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -758,10 +758,10 @@ en_ofctrl_is_connected_run(struct engine_node *node)
> > (struct ed_type_ofctrl_is_connected *)node->data;
> > if (data->connected != ofctrl_is_connected()) {
> > data->connected = !data->connected;
> > -node->changed = true;
> > +engine_set_node_state(node, EN_UPDATED);
> > return;
> > }
> > -node->changed = false;
> > +engine_set_node_state(node, EN_VALID);
> > }
> >
> > struct ed_type_addr_sets {
> > @@ -811,7 +811,7 @@ en_addr_sets_run(struct engine_node *node)
> > addr_sets_init(as_table, >addr_sets);
> >
> > as->change_tracked = false;
> > -node->changed = true;
> > +engine_set_node_state(node, EN_UPDATED);
> > }
> >
> > static bool
> > @@ -830,11 +830,14 @@ addr_sets_sb_address_set_handler(struct engine_node
> > *node)
> > addr_sets_update(as_table, >addr_sets, >new,
> > >deleted, >updated);
> >
> > -node->changed = !sset_is_empty(>new) ||
> > !sset_is_empty(>deleted)
> > -|| !sset_is_empty(>updated);
> > +if (!sset_is_empty(>new) || !sset_is_empty(>deleted) ||
> > +!sset_is_empty(>updated)) {
> > +engine_set_node_state(node, EN_UPDATED);
> > +} else {
> > +engine_set_node_state(node, EN_VALID);
> > +}
> >
> > as->change_tracked = true;
> > -node->changed = true;
> > return true;
> > }
> >
> > @@ -885,7 +888,7 @@ en_port_groups_run(struct engine_node *node)
> > port_groups_init(pg_table, >port_groups);
> >
> > pg->change_tracked = false;
> > -node->changed = true;
> > +engine_set_node_state(node, EN_UPDATED);
> > }
> >
> > static bool
> > @@ -904,11 +907,14 @@ port_groups_sb_port_group_handler(struct engine_node
> > *node)
> > port_groups_update(pg_table, >port_groups, >new,
> > >deleted, >updated);
> >
> > -node->changed = !sset_is_empty(>new) ||
> > !sset_is_empty(>deleted)
> > -|| !sset_is_empty(>updated);
> > +if (!sset_is_empty(>new) || !sset_is_empty(>deleted) ||
> > +!sset_is_empty(>updated)) {
> > +engine_set_node_state(node, EN_UPDATED);
> > +} else {
> > +engine_set_node_state(node, EN_VALID);
> > +}
> >
> > pg->change_tracked = true;
> > -node->changed = true;
> > return true;
> > }
> >
> > @@ -1091,7 +1097,7 @@ en_runtime_data_run(struct engine_node *node)
> > update_ct_zones(local_lports, local_datapaths, ct_zones,
> > ct_zone_bitmap, pending_ct_zones);
> >
> > -node->changed = true;
> > +engine_set_node_state(node, EN_UPDATED);
> > }
> >
> > static bool
> > @@ -1157,10
Re: [ovs-dev] [PATCH v3 ovn 1/4] ovn-controller: Refactor I-P engine_run() tracking.
On Mon, Nov 18, 2019 at 8:39 AM Numan Siddique wrote:
>
> On Thu, Nov 14, 2019 at 10:39 PM Dumitru Ceara wrote:
> >
> > This commit simplifies the logic of calling engine_run and engine_need_run
> > in
> > order to reduce the number of external variables required to track the
> > result
> > of the last engine execution.
> >
> > The engine code is also refactored a bit and the engine_run() function is
> > split in different functions that handle computing/recomputing a node.
> >
> > Signed-off-by: Dumitru Ceara
> > ---
> > controller/ovn-controller.c | 33 ++-
> > lib/inc-proc-eng.c | 124
> > +--
> > lib/inc-proc-eng.h |7 ++
> > 3 files changed, 107 insertions(+), 57 deletions(-)
> >
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 9ab98be..3922f3d 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -1942,7 +1942,6 @@ main(int argc, char *argv[])
> > _pkt);
> >
> > uint64_t engine_run_id = 0;
> > -uint64_t old_engine_run_id = 0;
> > bool engine_run_done = true;
> >
> > unsigned int ovs_cond_seqno = UINT_MAX;
> > @@ -1952,10 +1951,11 @@ main(int argc, char *argv[])
> > exiting = false;
> > restart = false;
> > while (!exiting) {
> > +engine_run_id++;
> > +
> > update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl);
> > update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl));
> >
> > ofctrl_set_probe_interval(get_ofctrl_probe_interval(ovs_idl_loop.idl));
> > -old_engine_run_id = engine_run_id;
> >
> > struct ovsdb_idl_txn *ovs_idl_txn =
> > ovsdb_idl_loop_run(_idl_loop);
> > unsigned int new_ovs_cond_seqno
> > @@ -2047,12 +2047,12 @@ main(int argc, char *argv[])
> > if (engine_run_done) {
> > engine_set_abort_recompute(true);
> > engine_run_done =
> > engine_run(_flow_output,
> > -
> > ++engine_run_id);
> > +
> > engine_run_id);
> > }
> > } else {
> > engine_set_abort_recompute(false);
> > engine_run_done = true;
> > -engine_run(_flow_output, ++engine_run_id);
> > +engine_run(_flow_output, engine_run_id);
> > }
> > }
> > stopwatch_stop(CONTROLLER_LOOP_STOPWATCH_NAME,
> > @@ -2095,17 +2095,20 @@ main(int argc, char *argv[])
> > }
> >
> > }
> > -if (old_engine_run_id == engine_run_id || !engine_run_done) {
> > -if (!engine_run_done || engine_need_run(_flow_output)) {
> > -VLOG_DBG("engine did not run, force recompute next
> > time: "
> > - "br_int %p, chassis %p", br_int, chassis);
> > -engine_set_force_recompute(true);
> > -poll_immediate_wake();
> > -} else {
> > -VLOG_DBG("engine did not run, and it was not needed"
> > - " either: br_int %p, chassis %p",
> > - br_int, chassis);
> > -}
> > +if (engine_need_run(_flow_output, engine_run_id)) {
> > +VLOG_DBG("engine did not run, force recompute next time: "
> > +"br_int %p, chassis %p", br_int, chassis);
> > +engine_set_force_recompute(true);
> > +poll_immediate_wake();
> > +} else if (!engine_run_done) {
> > +VLOG_DBG("engine was aborted, force recompute next time: "
> > + "br_int %p, chassis %p", br_int, chassis);
> > +engine_set_force_recompute(true);
> > +poll_immediate_wake();
> > +} else if (!engine_has_run(_flow_output, engine_run_id)) {
> > +VLOG_DBG("engine did not run, and it was not needed"
> > + " either: br_int %p, chassis %p",
> > + br_int, chassis);
> > } else {
> > engine_set_force_recompute(false);
> > }
> > diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c
> > index 1064a08..8a085e2 100644
> > --- a/lib/inc-proc-eng.c
> > +++ b/lib/inc-proc-eng.c
> > @@ -129,14 +129,72 @@ engine_ovsdb_node_add_index(struct engine_node *node,
> > const char *name,
> > }
> >
> > bool
> > -engine_run(struct engine_node *node, uint64_t run_id)
> > +engine_has_run(struct engine_node *node, uint64_t run_id)
> > +{
> > +return node->run_id == run_id;
> > +}
> > +
> > +/* Do a full recompute (or at
