[ovs-dev] Kreditangebot zwischen Privatpersonen.
Hallo Herr und Frau Kreditangebot zwischen Privatpersonen. Lassen Sie die Banken die Zinssätze nicht vorschreiben. Um Ihnen zu helfen, verschiedene finanzielle Probleme zu überwinden und Ihre Ziele für die Zukunft in einem Kontext ohne zu erreichen Schwierigkeitsgrad: Ich lege ein Darlehensangebot (Einzelpersonen, Unternehmen, Vereinigungen) in Schwierigkeiten vor oder möchte Hilfe bei der Lösung einiger finanzieller Probleme oder bei der Gründung eines Unternehmens. - Wahl des Betrags zwischen 5 000 und 13 000 000 € - Wahl der Rückzahlungsfrist: maximal 30 Jahre. - Jährlicher Rückzahlungszinssatz von 3%. Bitte mailen Sie mir in diesem Sinne oder kontaktieren Sie mich per E-Mail, wenn Sie an meinem Angebot interessiert sind. abby.patrici...@gmail.com abby.patrici...@gmail.com ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 0/3] Associate identifier with OVN ACL connection tracking entry
Hi Numan, Sure, i will submit a v5 soon. Regards, Ankur From: Numan Siddique Sent: Thursday, November 21, 2019 3:07 AM To: Ankur Sharma Cc: ovs-dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH v4 0/3] Associate identifier with OVN ACL connection tracking entry On Sat, Nov 9, 2019 at 8:20 AM Ankur Sharma wrote: > > I submitted this patch long time back and somehow lost track it. > Resubmitting the series, calling it as V4, as it addresses the > review comments given till v3. > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddev_2019-2DApril_358280.html=DwIBaQ=s883GpUCOChKOHiocYtGcg=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY=lJy4S1qKbRVivfyfmInKh6pOm1jqpisaSfNyBi90Avg=Sa7nACYuFbbSQiI4wVbJgTNMbhoX6bcB-ygpO0vnAvo= > > What: > > a. Goal is to be able to associate some identifier with a connection tracking > entry. > > b. This identifier can be used to map OVN ACL which added this entry or > higher level constructs like openstack security group etc. > > c. There are 2 connection tracking fields which can be used for it. > ct.mark (32 bits) and ct.label (128 bits). > > d. Patch intends to use ct.label, as this is a longer field and > hence would be put to a better use, if it stores the identifier. > > Why: > > a. Adding an identifier would help in debugging. > b. Now, we can map a connection tracking entry to corresponding >acl, security group etc. > c. One of the use cases for this mapping would be to identify >ACLs which added corresponding connection tracker entry, which >is causing unexpected drops/leaks. > > How: > > Following is the sequence of changes: > > Patch 1: >i. Current implementation uses a bit ct.label to handle policy update > cases, >where we use a bit in ct.label to indicate that reply traffic should >be dropped now. > ii. Swap the usage of ct.label in current implementation with ct.mark. > > Patch 2: >i. Add support in parser to allow ct.label and mark to be set from > registers > as well (as of now only integer/masked integer is allowed). > > Patch 3: >i. Add a new column (named 'label') to Table ACL in northbound schema. > ii. ovn-northd changes to enhance logical flows to set ct.label to > acl->label. > For example: > table=4 (ls_out_acl ), action=(reg0[1] = 1; reg0[3] = 1; > xxreg1 = 0x1234; next;) > . > . > . > table=7 (ls_out_stateful), ... match=(reg0[1] == 1 && reg0[3] == 1), > > Hi Ankur, Overall the series looks good to me. Can you please rebase and submit v5. In patch 2, using register offsets is not supported. Something like below "ct_commit(ct_label=xreg1[10..30]);" I would suggest to support that case as well. Thanks Numan > Ankur Sharma (3): > OVN ACL: Replace the usage of ct_label with ct_mark > OVN ACL: Allow ct_mark and ct_label values to be set from register as > well > OVN ACL: Allow a user to input ct.label value for an acl > > Documentation/tutorials/ovn-openstack.rst | 14 ++--- > include/ovn/actions.h | 3 + > lib/actions.c | 72 ++ > lib/logical-fields.c | 3 + > northd/ovn-northd.8.xml | 14 ++--- > northd/ovn-northd.c | 87 +-- > ovn-nb.ovsschema | 5 +- > ovn-nb.xml| 12 > ovn-sb.xml| 41 + > tests/ovn-nbctl.at| 12 +++- > tests/ovn.at | 99 > +-- > utilities/ovn-nbctl.c | 24 +++- > 12 files changed, 310 insertions(+), 76 deletions(-) > > -- > 1.8.3.1 > > ___ > dev mailing list > d...@openvswitch.org > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwIBaQ=s883GpUCOChKOHiocYtGcg=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY=lJy4S1qKbRVivfyfmInKh6pOm1jqpisaSfNyBi90Avg=8bIoE5Ex1Np-F8mBWnsmQzn1d0aO61JWUuAKNBHRS0Y= ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCHv5] netdev-afxdp: Enable loading XDP program.
Now netdev-afxdp always forwards all packets to userspace because
it is using libbpf's default XDP program, see 'xsk_load_xdp_prog'.
There are some cases when users want to keep packets in kernel instead
of sending to userspace, for example, management traffic such as SSH
should be processed in kernel.
The patch enables loading the user-provided XDP program by
$ovs-vsctl -- set int afxdp-p0 options:xdp-obj=
So users can implement their filtering logic or traffic steering idea
in their XDP program, and rest of the traffic passes to AF_XDP socket
handled by OVS.
Signed-off-by: William Tu
---
v5:
- rebase to master
Feedbacks from Eelco:
- Remove xdp-obj="__default__" case, to remove xdp-obj, use
ovs-vsctl remove int options xdp-obj
- Fix problem of xdp program not unloading
verify by bpftool.
- use xdp-obj instead of xdpobj
- Limitation: xdp-obj doesn't work when using best-effort-mode
because best-effort mode tried to probe mode by setting up queue,
and loading xdp-obj requires knwoing mode in advance.
(to support it, we might need to use the
XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD as in v3)
Testing
- I place two xdp binary here
https://drive.google.com/open?id=1QCCdNE-5CwlKCFV6Upg9mOPnnbVkUwA5
[xdpsock_pass.o] Working one, which forwards packets to dpif-netdev
[xdpsock_invalid.o] invalid one, which has no map
v4:
Feedbacks from Eelco.
- First load the program, then configure xsk.
Let API take care of xdp prog and map loading, don't set
XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD.
- When loading custom xdp, need to close(prog_fd) and close(map_fd)
to release the resources
- make sure prog and map is unloaded by bpftool.
- update doc, afxdp.rst
- Tested-at: https://travis-ci.org/williamtu/ovs-travis/builds/608986781
v3:
Feedbacks from Eelco.
- keep using xdpobj not xdp-obj (because we alread use xdpmode)
or we change both to xdp-obj and xdp-mode?
- log a info message when using external program for better debugging
- combine some failure messages
- update doc
NEW:
- add options:xdpobj=__default__, to set back to libbpf default prog
- Tested-at: https://travis-ci.org/williamtu/ovs-travis/builds/606153231
v2:
A couple fixes and remove RFC
---
Documentation/intro/install/afxdp.rst | 59 +++
NEWS | 2 +
lib/netdev-afxdp.c| 139 --
lib/netdev-linux-private.h| 4 +
4 files changed, 197 insertions(+), 7 deletions(-)
diff --git a/Documentation/intro/install/afxdp.rst
b/Documentation/intro/install/afxdp.rst
index 15e3c918f942..e72bb3edabe6 100644
--- a/Documentation/intro/install/afxdp.rst
+++ b/Documentation/intro/install/afxdp.rst
@@ -283,6 +283,65 @@ Or, use OVS pmd tool::
ovs-appctl dpif-netdev/pmd-stats-show
+Loading Custom XDP Program
+--
+By defailt, netdev-afxdp always forwards all packets to userspace because
+it is using libbpf's default XDP program. There are some cases when users
+want to keep packets in kernel instead of sending to userspace, for example,
+management traffic such as SSH should be processed in kernel. This can be
+done by loading the user-provided XDP program::
+
+ ovs-vsctl -- set int afxdp-p0 options:xdp-obj=
+
+So users can implement their filtering logic or traffic steering idea
+in their XDP program, and rest of the traffic passes to AF_XDP socket
+handled by OVS. To set it back to default, use::
+
+ ovs-vsctl remove int afxdp-p0 options xdp-obj
+
+Below is a sample C program compiled under kernel's samples/bpf/.
+
+.. code-block:: c
+
+ #include
+ #include "bpf_helpers.h"
+
+ #if LINUX_VERSION_CODE < KERNEL_VERSION(5,3,0)
+ /* Kernel version before 5.3 needed an additional map */
+ struct bpf_map_def SEC("maps") qidconf_map = {
+ .type = BPF_MAP_TYPE_ARRAY,
+ .key_size = sizeof(int),
+ .value_size = sizeof(int),
+ .max_entries = 64,
+ };
+ #endif
+
+ /* OVS will associate map 'xsks_map' to xsk socket. */
+ struct bpf_map_def SEC("maps") xsks_map = {
+ .type = BPF_MAP_TYPE_XSKMAP,
+ .key_size = sizeof(int),
+ .value_size = sizeof(int),
+ .max_entries = 32,
+ };
+
+ SEC("xdp_sock")
+ int xdp_sock_prog(struct xdp_md *ctx)
+ {
+ int index = ctx->rx_queue_index;
+
+ /* Customized by user.
+ * For example
+ * 1) filter out all SSH traffic and return XDP_PASS
+ *for kernel to process.
+ * 2) Drop unwanted packet by returning XDP_DROP.
+ */
+
+ /* Rest of packets goes to AF_XDP. */
+ return bpf_redirect_map(_map, index, 0);
+ }
+ char _license[] SEC("license") = "GPL";
+
+
Example Script
--
diff --git a/NEWS b/NEWS
index 100d7b6a8c29..d5b3063fb606 100644
--- a/NEWS
+++ b/NEWS
@@ -18,6 +18,8 @@ Post-v2.12.0
generic - former SKB
Re: [ovs-dev] [PATCH ovn] ovn-controller: Consider non-virtual ports first when updating bindings.
Acked-by: Mark Michelson
On 11/22/19 9:22 AM, Dumitru Ceara wrote:
There's no guarantee SBREC_PORT_BINDING_TABLE_FOR_EACH will first
return the non-virtual ports and then virtual ports. In the case when a
virtual port is processed before its virtual_parent,
consider_local_datapath might not release it in the current
ovn-controller iteration even though the virtual_parent gets released.
Right now this doesn't trigger any functionality issue because releasing
the virtual_parent triggers a change in the SB DB which will wake up
ovn-controller and trigger a new computation which will also update the
virtual port.
However, this is suboptimal, and we can notice that often ovn-controller
gets the SB update notification before the "transaction successful"
notification. In such cases the incremental engine doesn't run
(ovnsb_idl_txn == NULL) and a full recompute is scheduled for the next
run. By batching the two SB updates in a single transaction we
lower the risk of this situation happening.
CC: Numan Siddique
Fixes: 054f4c85c413 ("Add a new logical switch port type - 'virtual'")
Signed-off-by: Dumitru Ceara
---
controller/binding.c | 97 +---
1 file changed, 69 insertions(+), 28 deletions(-)
diff --git a/controller/binding.c b/controller/binding.c
index aad9d39..4c107c1 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -472,7 +472,12 @@ is_our_chassis(const struct sbrec_chassis *chassis_rec,
return our_chassis;
}
-static void
+/* Updates 'binding_rec' and if the port binding is local also updates the
+ * local datapaths and ports.
+ * Updates and returns the array of local virtual ports that will require
+ * additional processing.
+ */
+static const struct sbrec_port_binding **
consider_local_datapath(struct ovsdb_idl_txn *ovnsb_idl_txn,
struct ovsdb_idl_txn *ovs_idl_txn,
struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
@@ -485,7 +490,9 @@ consider_local_datapath(struct ovsdb_idl_txn *ovnsb_idl_txn,
struct hmap *local_datapaths,
struct shash *lport_to_iface,
struct sset *local_lports,
-struct sset *local_lport_ids)
+struct sset *local_lport_ids,
+const struct sbrec_port_binding **vpbs,
+size_t *n_vpbs, size_t *n_allocated_vpbs)
{
const struct ovsrec_interface *iface_rec
= shash_find_data(lport_to_iface, binding_rec->logical_port);
@@ -587,22 +594,11 @@ consider_local_datapath(struct ovsdb_idl_txn
*ovnsb_idl_txn,
}
} else if (binding_rec->chassis == chassis_rec) {
if (!strcmp(binding_rec->type, "virtual")) {
-/* pinctrl module takes care of binding the ports
- * of type 'virtual'.
- * Release such ports if their virtual parents are no
- * longer claimed by this chassis. */
-const struct sbrec_port_binding *parent
-= lport_lookup_by_name(sbrec_port_binding_by_name,
-binding_rec->virtual_parent);
-if (!parent || parent->chassis != chassis_rec) {
-VLOG_INFO("Releasing lport %s from this chassis.",
-binding_rec->logical_port);
-if (binding_rec->encap) {
-sbrec_port_binding_set_encap(binding_rec, NULL);
-}
-sbrec_port_binding_set_chassis(binding_rec, NULL);
-sbrec_port_binding_set_virtual_parent(binding_rec, NULL);
+if (*n_vpbs == *n_allocated_vpbs) {
+vpbs = x2nrealloc(vpbs, n_allocated_vpbs, sizeof *vpbs);
}
+vpbs[(*n_vpbs)] = binding_rec;
+(*n_vpbs)++;
} else {
VLOG_INFO("Releasing lport %s from this chassis.",
binding_rec->logical_port);
@@ -621,6 +617,30 @@ consider_local_datapath(struct ovsdb_idl_txn
*ovnsb_idl_txn,
vif_chassis);
}
}
+return vpbs;
+}
+
+static void
+consider_local_virtual_port(struct ovsdb_idl_index *sbrec_port_binding_by_name,
+const struct sbrec_chassis *chassis_rec,
+const struct sbrec_port_binding *binding_rec)
+{
+/* pinctrl module takes care of binding the ports of type 'virtual'.
+ * Release such ports if their virtual parents are no longer claimed by
+ * this chassis.
+ */
+const struct sbrec_port_binding *parent =
+lport_lookup_by_name(sbrec_port_binding_by_name,
+ binding_rec->virtual_parent);
+if (!parent || parent->chassis != chassis_rec) {
+VLOG_INFO("Releasing lport %s from this chassis.",
+
Re: [ovs-dev] [PATCH] ofproto-dpif: Expose datapath ND Extensions capability to ovsdb
On Fri, Nov 22, 2019 at 03:09:02PM -0300, Flavio Leitner wrote:
> Document and expose datapath ND Extensions capability to ovsdb.
>
> Fixes: d0d571493 ("ofproto-dpif: Allow IPv6 ND Extensions only if supported")
> Signed-off-by: Flavio Leitner
Thanks! Applied to master.
___
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 Fri, Nov 22, 2019 at 03:39:16PM -0500, Aaron Conole wrote:
> Marcelo Ricardo Leitner writes:
>
> > 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;
>
> But the maniptype of the first call could be NAT_MANIP_SRC. In fact,
> that's what I see if the packet is reply direction && !related.
Interesting, ok.
>
> So, we need the block to invert the manipulation type. Otherwise, we
> miss the DNAT manipulation.
>
> So I don't think I can use that block.
Thanks for digging on it.
Acked-by: Marcelo Ricardo Leitner
>
> >> >> + 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 2/2] act_ct: support asymmetric conntrack
Marcelo Ricardo Leitner writes:
> 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;
But the maniptype of the first call could be NAT_MANIP_SRC. In fact,
that's what I see if the packet is reply direction && !related.
So, we need the block to invert the manipulation type. Otherwise, we
miss the DNAT manipulation.
So I don't think I can use that block.
>> >> + 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] [ovs-discuss] [bug-report] Check Python IDL reconnects to leader unit case failed on arm64
Hi- I started looking at the same test failure on ppc64le. I found in my logs: "clustered database server has not yet joined cluster; trying another server" I found the same messsage in Lance's logs, so it could be the same issue. I made the following change to check that the cluster was up and running before running the test. diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at index cc38d69c1..5484fd4c5 100644 --- a/tests/ovsdb-idl.at +++ b/tests/ovsdb-idl.at @@ -1802,6 +1802,9 @@ m4_define([OVSDB_CHECK_IDL_LEADER_ONLY_PY], PARSE_LISTENING_PORT([s2.log], [TCP_PORT_1]) PARSE_LISTENING_PORT([s3.log], [TCP_PORT_2]) PARSE_LISTENING_PORT([s1.log], [TCP_PORT_3]) + OVS_WAIT_UNTIL([../../../ovsdb/ovsdb-tool check-cluster ./s1.db]) + OVS_WAIT_UNTIL([../../../ovsdb/ovsdb-tool check-cluster ./s2.db]) + OVS_WAIT_UNTIL([../../../ovsdb/ovsdb-tool check-cluster ./s3.db]) remotes=tcp:LPBK:$TCP_PORT_1,tcp:LPBK:$TCP_PORT_2,tcp:LPBK:$TCP_PORT_3 pids=$(cat s2.pid s3.pid s1.pid | tr '\n' ',') echo $pids This corrected the problem initially, however on Travis the problem returned intermittently, so I did not submit until I could look further. I hope this helps. Regards David Wilder On 2019-11-21 17:51, Lance Yang (Arm Technology China) wrote: Hi Ben, Thanks for your reply. I checked the original email I sent. I did attach the logs in my previous email. However, as you said, they might be stripped automatically. At this time, I compress everything into a zip file. I am not sure whether it would be stripped again. So I also attach a link where you can find the related logs: https://urldefense.proofpoint.com/v2/url?u=https-3A__1drv.ms_u_s-21Atj0ulDEFFyaj1ds2QyEmPMkRM9s-3Fe-3DIfr7l9=DwIFAg=jf_iaSHvJObTbx-siA1ZOg=7ndxyKjH9UrBD68Us2WP1wI4BwEBQbeAyz8i_vwCCaI=s9G46xQhkGSnSnZ64KeJqqxbxW8T4x8oC7cIXwZmm68=tWpvdvTFaHLkSQpOqJO8B0Qtj6runK-eQvnxwsAhz18= . For convenience, I also copied the original email content as below in case that you did not receive the complete message: I encountered an issue when I was running testsuite on Arm. The system I tested on was an arm server running Ubuntu 18.04.3 LTS (GNU/Linux 4.15.0-65-generic aarch64). After I downloaded the up-to-date source code on master branch and built ovs, I run the testsuite with TESTSUITEFLAGS="2105" (Unit case name: Check Python IDL reconnects to leader) I run the unit case for many times, most of the time I got the following failure: make check TESTSUITEFLAGS="2105" RECHECK=yes make check-recursive make[1]: Entering directory '/home/lance/ovs' Making check in datapath make[2]: Entering directory '/home/lance/ovs/datapath' make[3]: Entering directory '/home/lance/ovs/datapath' make[3]: Leaving directory '/home/lance/ovs/datapath' make[2]: Leaving directory '/home/lance/ovs/datapath' make[2]: Entering directory '/home/lance/ovs' make[3]: Entering directory '/home/lance/ovs/datapath' make[3]: 'distfiles' is up to date. make[3]: Leaving directory '/home/lance/ovs/datapath' make utilities/ovs-appctl-bashcomp.bash utilities/ovs-vsctl-bashcomp.bash tests/atlocal make[3]: Entering directory '/home/lance/ovs' make[3]: Nothing to be done for 'utilities/ovs-appctl-bashcomp.bash'. make[3]: Nothing to be done for 'utilities/ovs-vsctl-bashcomp.bash'. make[3]: 'tests/atlocal' is up to date. make[3]: Leaving directory '/home/lance/ovs' make check-local make[3]: Entering directory '/home/lance/ovs' set /bin/bash './tests/testsuite' -C tests AUTOTEST_PATH=utilities:vswitchd:ovsdb:vtep:tests::; \ "$@" -j2 2104-2105 || (test X'' = Xyes && "$@" --recheck) ## --- ## ## openvswitch 2.12.90 test suite. ## ## --- ## 2105: Check Python IDL reconnects to leader - Python3 (leader only) FAILED (ovsdb-idl.at:1816) ## - ## ## Test results. ## ## - ## ERROR: All 1 test was run, 1 failed unexpectedly. ## -- ## ## testsuite.log was created. ## ## -- ## Please send `tests/testsuite.log' and all information you think might help: To: Subject: [openvswitch 2.12.90] testsuite: 2105 failed You may investigate any problem if you feel able to do so, in which case the test suite provides a good starting point. Its output may be found below `tests/testsuite.dir'. ## --- ## 2105: Check Python IDL reconnects to leader - Python3 (leader only) FAILED (ovsdb-idl.at:1816) ## - ## ## Test results. ## ## - ## ERROR: 1 test was run, 1 failed unexpectedly. ## -- ## ## testsuite.log was created. ## ## -- ## Please send `tests/testsuite.log' and all information you think might help: To: Subject: [openvswitch 2.12.90] testsuite: 2105 failed You may investigate any problem if you feel able to do so, in which case the test suite provides a good starting point. Its output may be found below `tests/testsuite.dir'.
Re: [ovs-dev] [PATCH ovn v3] Require Python 3 and remove support for Python 2.
Acked-by: Mark Michelson
On 11/22/19 12:21 PM, num...@ovn.org wrote:
From: Numan Siddique
OVS removed the support for Python 2 in the commit [1].
And its time we do the same for OVN. Python3 is now mandatory,
otherwise configure will fail.
This patch takes care of removing Python 2 references.
[1] - 1ca0323e7c29("Require Python 3 and remove support for Python 2.")
Signed-off-by: Numan Siddique
---
v2 -> v3
=
* Rebased to the latest master
v1 -> v2
===
* Addressed the review comments from Mark - Removed HAVE_PYTHON3 var
and fixed ovn-detrace.in exception seen with python3.
Makefile.am | 12 ++--
automake.mk | 8 +--
build-aux/dpdkstrip.py | 2 +-
build-aux/sodepends.py | 2 +-
build-aux/soexpand.py | 2 +-
configure.ac| 2 -
ipsec/ovs-monitor-ipsec.in | 2 +-
m4/ovn.m4 | 89 ++---
rhel/ovn-fedora.spec.in | 35 +-
tests/atlocal.in| 23 ++-
tests/checkpatch.at | 12 ++--
tests/ovn-controller-vtep.at| 1 -
tests/ovn-northd.at | 10 ---
tests/ovn.at| 63 -
tests/ovsdb-macros.at | 4 --
tests/system-kmod-macros.at | 1 -
tests/system-userspace-macros.at| 1 -
tutorial/ovs-sandbox| 1 +
utilities/bugtool/automake.mk | 2 -
utilities/checkpatch.py | 3 +-
utilities/ovn-detrace.in| 6 +-
utilities/ovn-docker-overlay-driver.in | 2 +-
utilities/ovn-docker-underlay-driver.in | 2 +-
23 files changed, 36 insertions(+), 249 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 1e41e49ea..8eed7a72b 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -71,7 +71,7 @@ endif
# foo/__init__.pyc will cause Python to ignore foo.py.
run_python = \
PYTHONPATH=$(OVS_SRCDIR)/python$(psep)$$PYTHONPATH \
- PYTHONDONTWRITEBYTECODE=yes $(PYTHON)
+ PYTHONDONTWRITEBYTECODE=yes $(PYTHON3)
ALL_LOCAL =
BUILT_SOURCES =
@@ -165,13 +165,13 @@ ro_shell = printf '\043 Generated automatically -- do not
modify!-*- buffer-
SUFFIXES += .in
.in:
-
$(AM_V_GEN)PYTHONPATH=$(OVS_SRCDIR)/python$(psep)$$PYTHONPATH$(psep)$(srcdir)/python
$(PYTHON) $(srcdir)/build-aux/soexpand.py -I$(srcdir) -I$(OVS_SRCDIR) < $< | \
- $(PYTHON) $(srcdir)/build-aux/dpdkstrip.py $(DPDKSTRIP_FLAGS) | \
+
$(AM_V_GEN)PYTHONPATH=$(OVS_SRCDIR)/python$(psep)$$PYTHONPATH$(psep)$(srcdir)/python
$(PYTHON3) $(srcdir)/build-aux/soexpand.py -I$(srcdir) -I$(OVS_SRCDIR) < $< | \
+ $(PYTHON3) $(srcdir)/build-aux/dpdkstrip.py $(DPDKSTRIP_FLAGS) | \
sed \
-e 's,[@]PKIDIR[@],$(PKIDIR),g' \
-e 's,[@]LOGDIR[@],$(LOGDIR),g' \
-e 's,[@]DBDIR[@],$(DBDIR),g' \
- -e 's,[@]PYTHON[@],$(PYTHON),g' \
+ -e 's,[@]PYTHON3[@],$(PYTHON3),g' \
-e 's,[@]OVN_RUNDIR[@],$(OVN_RUNDIR),g' \
-e 's,[@]OVSBUILDDIR[@],$(OVSBUILDDIR),g' \
-e 's,[@]VERSION[@],$(VERSION),g' \
@@ -197,7 +197,7 @@ SUFFIXES += .xml
PKIDIR='$(PKIDIR)' \
LOGDIR='$(LOGDIR)' \
DBDIR='$(DBDIR)' \
- PYTHON='$(PYTHON)' \
+ PYTHON3='$(PYTHON3)' \
RUNDIR='$(RUNDIR)' \
OVN_RUNDIR='$(OVN_RUNDIR)' \
VERSION='$(VERSION)' \
@@ -425,7 +425,7 @@ CLEANFILES += flake8-check
include $(srcdir)/manpages.mk
$(srcdir)/manpages.mk: $(MAN_ROOTS) build-aux/sodepends.py
$(OVS_SRCDIR)/python/build/soutil.py
- @PYTHONPATH=$(OVS_SRCDIR)/python$(psep)$$PYTHONPATH$(psep)$(srcdir)/python
$(PYTHON) $(srcdir)/build-aux/sodepends.py -I. -I$(srcdir) -I$(OVS_MANDIR)
$(MAN_ROOTS) >$(@F).tmp
+ @PYTHONPATH=$(OVS_SRCDIR)/python$(psep)$$PYTHONPATH$(psep)$(srcdir)/python
$(PYTHON3) $(srcdir)/build-aux/sodepends.py -I. -I$(srcdir) -I$(OVS_MANDIR)
$(MAN_ROOTS) >$(@F).tmp
@if cmp -s $(@F).tmp $@; then \
touch $@; \
rm -f $(@F).tmp; \
diff --git a/automake.mk b/automake.mk
index ad801f1e5..591e00751 100644
--- a/automake.mk
+++ b/automake.mk
@@ -6,19 +6,17 @@ CLEANFILES += ovn-architecture.7
#
# If "python" or "dot" is not available, then we do not add graphical diagram
# to the documentation.
-if HAVE_PYTHON
if HAVE_DOT
OVSDB_DOT = $(run_python) ${OVSDIR}/ovsdb/ovsdb-dot.in
ovn-nb.gv: ${OVSDIR}/ovsdb/ovsdb-dot.in $(srcdir)/ovn-nb.ovsschema
$(AM_V_GEN)$(OVSDB_DOT) --no-arrows $(srcdir)/ovn-nb.ovsschema > $@
ovn-nb.pic: ovn-nb.gv ${OVSDIR}/ovsdb/dot2pic
- $(AM_V_GEN)(dot -T plain < ovn-nb.gv | $(PYTHON) ${OVSDIR}/ovsdb/dot2pic -f 3)
> $@.tmp && \
+ $(AM_V_GEN)(dot -T plain < ovn-nb.gv | $(PYTHON3) ${OVSDIR}/ovsdb/dot2pic -f 3)
> $@.tmp && \
Re: [ovs-dev] iperf tcp issue on veth using afxdp
On 22.11.2019 18:51, William Tu wrote: > Hi Ilya and Eelco, > > Yiyang reports very poor TCP performance on his setup and I can > also reproduce it on my machine. Somehow I think this might be a > kernel issue, but I don't know where to debug this. Need your suggestion > about how to debug. > > So the setup is like the system-traffic, creating 2 namespaces and > veth devices and attach to OVS. I do remember to turn off tx offload > and ping, UDP, nc (tcp-mode) works fine. > > TCP using iperf drops to 0Mbps after 4 seconds. The key questions are: * What is your kernel version? * Does it work in generic mode? * Does it work if iperf generates UDP traffic? ex. iperf3 -c 10.1.1.1 -t 3600 -u -b 10G/64 -l 1460 * It seems like a ring breakage or a umem memory leak. So, what are the afxdp related coverage counters? * Does OVS forward packets, i.e. if there something received/sent? Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2 2/2] northd: Improve handling of pause and resume
fre. 22. nov. 2019, 19:10 skrev Numan Siddique :
> On Fri, Nov 22, 2019 at 8:22 PM Frode Nordahl
> wrote:
> >
> > Move paused state to ``struct northd_context`` and pass the
> > context to paused and status command handlers.
> >
> > On pause release the OVSDB lock on SB DB.
> >
> > Re-instante the lock on resume.
> >
> > Status command will now provide accurate information for 'paused',
> > 'active' and 'standby' states.
> >
> > Merge separate status command test into the pause and resume tests.
> >
> > Signed-off-by: Frode Nordahl
>
> Hi Frode,
>
> Thanks for the patch.
>
Thank you for the review, Numan.
Using ctx in the ctl functions doesn't seem right to me.
> When the user runs "ovn-appctl -t ovn-northd pause/resume" and if
> ctx->ovnsb_idl is NULL,
> then ovn_northd_pause()/ovn_northd_resume() will just ignore these
> commands.
>
I don't actually expect the ``ctx->ovnsb_idl`` to ever be NULL. It's just a
bone marrow reaction to always check before handing off something that can
be dereferenced.
I think you can call ovsdb_idl_set_lock with the appropriate params in
> the main while loop itself
> depending on the value of "paused".
>
I see what you mean, and having calls on the idl outside the main loop may
quickly get us into trouble I guess.
The problem I'm left with is that the status command function would need
access to both the ``paused`` and ``had_lock`` states.
What would you think about putting those in a new struct that we pass to
the pause, resume and status command functions?
--
Frode Nordahl
Thanks
> Numan
>
> > ---
> > northd/ovn-northd.8.xml | 9 +++--
> > northd/ovn-northd.c | 87 +++--
> > tests/ovn-northd.at | 24 +++-
> > 3 files changed, 79 insertions(+), 41 deletions(-)
> >
> > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> > index 9734e79e6..c6d5d96b9 100644
> > --- a/northd/ovn-northd.8.xml
> > +++ b/northd/ovn-northd.8.xml
> > @@ -74,13 +74,15 @@
> >pause
> >
> > Pauses the ovn-northd operation from processing any Northbound
> and
> > -Southbound database changes.
> > +Southbound database changes. This will also instruct
> ovn-northd to
> > +drop any lock on SB DB.
> >
> >
> >resume
> >
> > Resumes the ovn-northd operation to process Northbound and
> > -Southbound database contents and generate logical flows.
> > +Southbound database contents and generate logical flows. This
> will
> > +also instruct ovn-northd to aspire for the lock on SB DB.
> >
> >
> >is-paused
> > @@ -91,7 +93,8 @@
> >status
> >
> > Prints this server's status. Status will be "active" if
> ovn-northd has
> > -acquired OVSDB lock on NB DB, "standby" otherwise.
> > +acquired OVSDB lock on SB DB, "standby" if it has not or
> "paused" if
> > +this instance is paused.
> >
> >
> >
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index a943e1037..e19515d14 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -65,6 +65,7 @@ struct northd_context {
> > struct ovsdb_idl_index *sbrec_ha_chassis_grp_by_name;
> > struct ovsdb_idl_index *sbrec_mcast_group_by_name_dp;
> > struct ovsdb_idl_index *sbrec_ip_mcast_by_dp;
> > +bool paused;
> > };
> >
> > /* An IPv4 or IPv6 address */
> > @@ -10836,6 +10837,8 @@ add_column_noalert(struct ovsdb_idl *idl,
> > ovsdb_idl_omit_alert(idl, column);
> > }
> >
> > +#define OVN_NORTHD_SB_DB_LOCK_NAME "ovn_northd"
> > +
> > int
> > main(int argc, char *argv[])
> > {
> > @@ -10843,8 +10846,10 @@ main(int argc, char *argv[])
> > struct unixctl_server *unixctl;
> > int retval;
> > bool exiting;
> > -bool paused;
> > bool had_lock;
> > +struct northd_context ctx;
> > +
> > +memset(, 0, sizeof(ctx));
> >
> > fatal_ignore_sigpipe();
> > ovs_cmdl_proctitle_init(argc, argv);
> > @@ -10866,11 +10871,11 @@ main(int argc, char *argv[])
> > exit(EXIT_FAILURE);
> > }
> > unixctl_command_register("exit", "", 0, 0, ovn_northd_exit,
> );
> > -unixctl_command_register("pause", "", 0, 0, ovn_northd_pause,
> );
> > -unixctl_command_register("resume", "", 0, 0, ovn_northd_resume,
> );
> > +unixctl_command_register("pause", "", 0, 0, ovn_northd_pause, );
> > +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);
> > + );
> > +unixctl_command_register("status", "", 0, 0, ovn_northd_status,
> );
> >
> > daemonize_complete();
> >
> > @@ -11075,23 +11080,21 @@ main(int argc, char *argv[])
> > /* Ensure that only a single ovn-northd is active in
Re: [ovs-dev] [PATCH] ofproto-dpif: Allow IPv6 ND Extensions only if supported
On Thu, 21 Nov 2019 17:22:49 -0800
Ben Pfaff wrote:
> On Wed, Nov 20, 2019 at 11:28:03AM -0300, Flavio Leitner wrote:
> > On Wed, 20 Nov 2019 11:21:13 -0300
> > Flavio Leitner wrote:
> >
> > > The IPv6 ND Extensions is only implemented in userspace datapath,
> > > but nothing prevents that to be used with other datapaths.
> > >
> > > This patch probes the datapath and only allows if the support
> > > is available.
> > >
> > > Fixes: 9b2b84973 ("Support for match & set ICMPv6 reserved and
> > > options type fields") Signed-off-by: Flavio Leitner
> > > ---
> >
> > This fix should be applied to branch-2.12 as well.
> > The same patch applies today, otherwise let me know and I can send
> > out the back-porting too.
>
> I applied this to master and branch-2.12.
>
> Since you posted this patch, William pushed commit 27501802d09f
> ("ofproto-dpif: Expose datapath capability to ovsdb.") that exposes
> datapath capabilities via OVSDB and, more importantly, documents the
> meanings of each of the capabilities. Would you mind sending a
> followup patch to add documentation for this new capability?
I posted a patch documenting and exposing it:
https://mail.openvswitch.org/pipermail/ovs-dev/2019-November/365078.html
Thanks Ben!
--
fbl
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2 2/2] northd: Improve handling of pause and resume
On Fri, Nov 22, 2019 at 8:22 PM Frode Nordahl
wrote:
>
> Move paused state to ``struct northd_context`` and pass the
> context to paused and status command handlers.
>
> On pause release the OVSDB lock on SB DB.
>
> Re-instante the lock on resume.
>
> Status command will now provide accurate information for 'paused',
> 'active' and 'standby' states.
>
> Merge separate status command test into the pause and resume tests.
>
> Signed-off-by: Frode Nordahl
Hi Frode,
Thanks for the patch.
Using ctx in the ctl functions doesn't seem right to me.
When the user runs "ovn-appctl -t ovn-northd pause/resume" and if
ctx->ovnsb_idl is NULL,
then ovn_northd_pause()/ovn_northd_resume() will just ignore these commands.
I think you can call ovsdb_idl_set_lock with the appropriate params in
the main while loop itself
depending on the value of "paused".
Thanks
Numan
> ---
> northd/ovn-northd.8.xml | 9 +++--
> northd/ovn-northd.c | 87 +++--
> tests/ovn-northd.at | 24 +++-
> 3 files changed, 79 insertions(+), 41 deletions(-)
>
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 9734e79e6..c6d5d96b9 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -74,13 +74,15 @@
>pause
>
> Pauses the ovn-northd operation from processing any Northbound and
> -Southbound database changes.
> +Southbound database changes. This will also instruct ovn-northd to
> +drop any lock on SB DB.
>
>
>resume
>
> Resumes the ovn-northd operation to process Northbound and
> -Southbound database contents and generate logical flows.
> +Southbound database contents and generate logical flows. This will
> +also instruct ovn-northd to aspire for the lock on SB DB.
>
>
>is-paused
> @@ -91,7 +93,8 @@
>status
>
> Prints this server's status. Status will be "active" if ovn-northd
> has
> -acquired OVSDB lock on NB DB, "standby" otherwise.
> +acquired OVSDB lock on SB DB, "standby" if it has not or "paused" if
> +this instance is paused.
>
>
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index a943e1037..e19515d14 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -65,6 +65,7 @@ struct northd_context {
> struct ovsdb_idl_index *sbrec_ha_chassis_grp_by_name;
> struct ovsdb_idl_index *sbrec_mcast_group_by_name_dp;
> struct ovsdb_idl_index *sbrec_ip_mcast_by_dp;
> +bool paused;
> };
>
> /* An IPv4 or IPv6 address */
> @@ -10836,6 +10837,8 @@ add_column_noalert(struct ovsdb_idl *idl,
> ovsdb_idl_omit_alert(idl, column);
> }
>
> +#define OVN_NORTHD_SB_DB_LOCK_NAME "ovn_northd"
> +
> int
> main(int argc, char *argv[])
> {
> @@ -10843,8 +10846,10 @@ main(int argc, char *argv[])
> struct unixctl_server *unixctl;
> int retval;
> bool exiting;
> -bool paused;
> bool had_lock;
> +struct northd_context ctx;
> +
> +memset(, 0, sizeof(ctx));
>
> fatal_ignore_sigpipe();
> ovs_cmdl_proctitle_init(argc, argv);
> @@ -10866,11 +10871,11 @@ main(int argc, char *argv[])
> exit(EXIT_FAILURE);
> }
> unixctl_command_register("exit", "", 0, 0, ovn_northd_exit, );
> -unixctl_command_register("pause", "", 0, 0, ovn_northd_pause, );
> -unixctl_command_register("resume", "", 0, 0, ovn_northd_resume, );
> +unixctl_command_register("pause", "", 0, 0, ovn_northd_pause, );
> +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);
> + );
> +unixctl_command_register("status", "", 0, 0, ovn_northd_status, );
>
> daemonize_complete();
>
> @@ -11075,23 +11080,21 @@ main(int argc, char *argv[])
> /* Ensure that only a single ovn-northd is active in the deployment by
> * 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");
> +ovsdb_idl_set_lock(ovnsb_idl_loop.idl, OVN_NORTHD_SB_DB_LOCK_NAME);
>
> /* Main loop. */
> exiting = false;
> -paused = false;
> +ctx.paused = false;
> had_lock = false;
> while (!exiting) {
> -if (!paused) {
> -struct northd_context ctx = {
> -.ovnnb_idl = ovnnb_idl_loop.idl,
> -.ovnnb_txn = ovsdb_idl_loop_run(_idl_loop),
> -.ovnsb_idl = ovnsb_idl_loop.idl,
> -.ovnsb_txn = ovsdb_idl_loop_run(_idl_loop),
> -.sbrec_ha_chassis_grp_by_name = sbrec_ha_chassis_grp_by_name,
> -.sbrec_mcast_group_by_name_dp =
[ovs-dev] [PATCH] ofproto-dpif: Expose datapath ND Extensions capability to ovsdb
Document and expose datapath ND Extensions capability to ovsdb.
Fixes: d0d571493 ("ofproto-dpif: Allow IPv6 ND Extensions only if supported")
Signed-off-by: Flavio Leitner
---
ofproto/ofproto-dpif.c | 1 +
vswitchd/vswitch.xml | 5 +
2 files changed, 6 insertions(+)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 5360e7209..2cd786a16 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5532,6 +5532,7 @@ get_datapath_cap(const char *datapath_type, struct smap
*cap)
smap_add(cap, "ct_state_nat", odp.ct_state_nat ? "true" : "false");
smap_add(cap, "ct_orig_tuple", odp.ct_orig_tuple ? "true" : "false");
smap_add(cap, "ct_orig_tuple6", odp.ct_orig_tuple6 ? "true" : "false");
+smap_add(cap, "nd_ext", odp.nd_ext ? "true" : "false");
/* DPIF_SUPPORT_FIELDS */
smap_add(cap, "masked_set_action", s.masked_set_action ? "true" : "false");
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index d4cc3b9ed..09e7bdf5f 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -5828,6 +5828,11 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch
options:peer=p1 \
packet to be sent to the Open vSwitch slow path, which is likely to
make it too slow for mirroring traffic in bulk.
+
+True if the datapath supports OVS_KEY_ATTR_ND_EXTENSIONS to match on
+ICMPv6 "ND reserved" and “ND option type“ header fields. If false,
+the datapath reports error if the feature is used.
+
When Open vSwitch translates actions from OpenFlow into the datapath
--
2.23.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v3] Require Python 3 and remove support for Python 2.
Bleep bloop. Greetings Numan Siddique, 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: error: Your local changes to the following files would be overwritten by merge: utilities/checkpatch.py Please, commit your changes or stash them before you can merge. Aborting Failed to merge in the changes. Patch failed at 0001 Require Python 3 and remove support for Python 2. 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] iperf tcp issue on veth using afxdp
Hi Ilya and Eelco, Yiyang reports very poor TCP performance on his setup and I can also reproduce it on my machine. Somehow I think this might be a kernel issue, but I don't know where to debug this. Need your suggestion about how to debug. So the setup is like the system-traffic, creating 2 namespaces and veth devices and attach to OVS. I do remember to turn off tx offload and ping, UDP, nc (tcp-mode) works fine. TCP using iperf drops to 0Mbps after 4 seconds. At server side: root@osboxes:~/ovs# ip netns exec at_ns0 iperf -s Server listening on TCP port 5001 TCP window size: 128 KByte (default) [ 4] local 10.1.1.1 port 5001 connected with 10.1.1.2 port 40384 Waiting for server threads to complete. Interrupt again to force quit. At client side root@osboxes:~/bpf-next# ip netns exec at_ns1 iperf -c 10.1.1.1 -i 1 -t 10 Client connecting to 10.1.1.1, TCP port 5001 TCP window size: 85.0 KByte (default) [ 3] local 10.1.1.2 port 40384 connected with 10.1.1.1 port 5001 [ ID] Interval Transfer Bandwidth [ 3] 0.0- 1.0 sec 17.0 MBytes 143 Mbits/sec [ 3] 1.0- 2.0 sec 9.62 MBytes 80.7 Mbits/sec [ 3] 2.0- 3.0 sec 6.75 MBytes 56.6 Mbits/sec [ 3] 3.0- 4.0 sec 11.0 MBytes 92.3 Mbits/sec [ 3] 5.0- 6.0 sec 0.00 Bytes 0.00 bits/sec [ 3] 6.0- 7.0 sec 0.00 Bytes 0.00 bits/sec [ 3] 7.0- 8.0 sec 0.00 Bytes 0.00 bits/sec [ 3] 8.0- 9.0 sec 0.00 Bytes 0.00 bits/sec [ 3] 9.0-10.0 sec 0.00 Bytes 0.00 bits/sec [ 3] 10.0-11.0 sec 0.00 Bytes 0.00 bits/sec (after this, even ping stops working) Script to reproduce - ovs-vsctl -- add-br br0 -- set Bridge br0 datapath_type=netdev ip netns add at_ns0 ip link add p0 type veth peer name afxdp-p0 ip link set p0 netns at_ns0 ip link set dev afxdp-p0 up ovs-vsctl add-port br0 afxdp-p0 ovs-vsctl -- set interface afxdp-p0 options:n_rxq=1 type="afxdp" options:xdp-mode=native ip netns exec at_ns0 sh << NS_EXEC_HEREDOC ip addr add "10.1.1.1/24" dev p0 ip link set dev p0 up NS_EXEC_HEREDOC ip netns add at_ns1 ip link add p1 type veth peer name afxdp-p1 ip link set p1 netns at_ns1 ip link set dev afxdp-p1 up ovs-vsctl add-port br0 afxdp-p1 -- \ set interface afxdp-p1 options:n_rxq=1 type="afxdp" options:xdp-mode=native ip netns exec at_ns1 sh << NS_EXEC_HEREDOC ip addr add "10.1.1.2/24" dev p1 ip link set dev p1 up NS_EXEC_HEREDOC ethtool -K afxdp-p0 tx off ethtool -K afxdp-p1 tx off ip netns exec at_ns0 ethtool -K p0 tx off ip netns exec at_ns1 ethtool -K p1 tx off ip netns exec at_ns0 ping -c 10 -i .2 10.1.1.2 echo "ip netns exec at_ns1 iperf -c 10.1.1.1 -i 1 -t 10" ip netns exec at_ns0 iperf -s Thank you William ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovs] ofproto-dpif-upcall: Allow HASH packet attribute
On Fri, Nov 15, 2019 at 10:58:59AM +0800, xiangxia.m@gmail.com wrote: > From: Tonghao Zhang > > The kernel datapath may sent upcall with hash info, > ovs-vswitchd should get it from upcall and then send > it back. Thanks. I applied this to master. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ofproto-dpif: Refactor the get capability code.
On Thu, Nov 21, 2019 at 11:09:02AM -0800, William Tu wrote: > Make the code simpler by removing the use of > xasprintf and free, and use smap_add_format. > > Cc: Ben Pfaff > Signed-off-by: William Tu Looks good to me. Acked-by: Ben Pfaff ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] Datapath: Change in openvswitch kernel module to support MPLS label depth of 3 in ingress direction.
On Fri, Nov 22, 2019 at 08:32:53AM -0800, Gregory Rose wrote: > On 11/21/2019 10:07 PM, Martin Varghese wrote: > > From: Martin Varghese > > > > Upstream commit: > > commit fbdcdd78da7c95f1b970d371e1b23cbd3aa990f3 > > Author: Martin Varghese > > Date: Mon Nov 4 07:27:44 2019 +0530 > > > > Change in Openvswitch to support MPLS label depth of 3 in ingress > > direction > > > > The openvswitch was supporting a MPLS label depth of 1 in the > > ingress direction though the userspace OVS supports a max depth > > of 3 labels.This change enables openvswitch module to support a > > max depth of 3 labels in the ingress. > > > > Signed-off-by: Martin Varghese > > Acked-by: Pravin B Shelar > > Signed-off-by: David S. Miller > > > > Signed-off-by: Martin Varghese > > Thanks Martin. > > Tested-by: Greg Rose > Reviewed-by: Greg Rose I applied this to master, thanks everyone! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v3] Require Python 3 and remove support for Python 2.
From: Numan Siddique
OVS removed the support for Python 2 in the commit [1].
And its time we do the same for OVN. Python3 is now mandatory,
otherwise configure will fail.
This patch takes care of removing Python 2 references.
[1] - 1ca0323e7c29("Require Python 3 and remove support for Python 2.")
Signed-off-by: Numan Siddique
---
v2 -> v3
=
* Rebased to the latest master
v1 -> v2
===
* Addressed the review comments from Mark - Removed HAVE_PYTHON3 var
and fixed ovn-detrace.in exception seen with python3.
Makefile.am | 12 ++--
automake.mk | 8 +--
build-aux/dpdkstrip.py | 2 +-
build-aux/sodepends.py | 2 +-
build-aux/soexpand.py | 2 +-
configure.ac| 2 -
ipsec/ovs-monitor-ipsec.in | 2 +-
m4/ovn.m4 | 89 ++---
rhel/ovn-fedora.spec.in | 35 +-
tests/atlocal.in| 23 ++-
tests/checkpatch.at | 12 ++--
tests/ovn-controller-vtep.at| 1 -
tests/ovn-northd.at | 10 ---
tests/ovn.at| 63 -
tests/ovsdb-macros.at | 4 --
tests/system-kmod-macros.at | 1 -
tests/system-userspace-macros.at| 1 -
tutorial/ovs-sandbox| 1 +
utilities/bugtool/automake.mk | 2 -
utilities/checkpatch.py | 3 +-
utilities/ovn-detrace.in| 6 +-
utilities/ovn-docker-overlay-driver.in | 2 +-
utilities/ovn-docker-underlay-driver.in | 2 +-
23 files changed, 36 insertions(+), 249 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 1e41e49ea..8eed7a72b 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -71,7 +71,7 @@ endif
# foo/__init__.pyc will cause Python to ignore foo.py.
run_python = \
PYTHONPATH=$(OVS_SRCDIR)/python$(psep)$$PYTHONPATH \
- PYTHONDONTWRITEBYTECODE=yes $(PYTHON)
+ PYTHONDONTWRITEBYTECODE=yes $(PYTHON3)
ALL_LOCAL =
BUILT_SOURCES =
@@ -165,13 +165,13 @@ ro_shell = printf '\043 Generated automatically -- do not
modify!-*- buffer-
SUFFIXES += .in
.in:
-
$(AM_V_GEN)PYTHONPATH=$(OVS_SRCDIR)/python$(psep)$$PYTHONPATH$(psep)$(srcdir)/python
$(PYTHON) $(srcdir)/build-aux/soexpand.py -I$(srcdir) -I$(OVS_SRCDIR) < $< | \
- $(PYTHON) $(srcdir)/build-aux/dpdkstrip.py $(DPDKSTRIP_FLAGS) | \
+
$(AM_V_GEN)PYTHONPATH=$(OVS_SRCDIR)/python$(psep)$$PYTHONPATH$(psep)$(srcdir)/python
$(PYTHON3) $(srcdir)/build-aux/soexpand.py -I$(srcdir) -I$(OVS_SRCDIR) < $< | \
+ $(PYTHON3) $(srcdir)/build-aux/dpdkstrip.py $(DPDKSTRIP_FLAGS) | \
sed \
-e 's,[@]PKIDIR[@],$(PKIDIR),g' \
-e 's,[@]LOGDIR[@],$(LOGDIR),g' \
-e 's,[@]DBDIR[@],$(DBDIR),g' \
- -e 's,[@]PYTHON[@],$(PYTHON),g' \
+ -e 's,[@]PYTHON3[@],$(PYTHON3),g' \
-e 's,[@]OVN_RUNDIR[@],$(OVN_RUNDIR),g' \
-e 's,[@]OVSBUILDDIR[@],$(OVSBUILDDIR),g' \
-e 's,[@]VERSION[@],$(VERSION),g' \
@@ -197,7 +197,7 @@ SUFFIXES += .xml
PKIDIR='$(PKIDIR)' \
LOGDIR='$(LOGDIR)' \
DBDIR='$(DBDIR)' \
- PYTHON='$(PYTHON)' \
+ PYTHON3='$(PYTHON3)' \
RUNDIR='$(RUNDIR)' \
OVN_RUNDIR='$(OVN_RUNDIR)' \
VERSION='$(VERSION)' \
@@ -425,7 +425,7 @@ CLEANFILES += flake8-check
include $(srcdir)/manpages.mk
$(srcdir)/manpages.mk: $(MAN_ROOTS) build-aux/sodepends.py
$(OVS_SRCDIR)/python/build/soutil.py
-
@PYTHONPATH=$(OVS_SRCDIR)/python$(psep)$$PYTHONPATH$(psep)$(srcdir)/python
$(PYTHON) $(srcdir)/build-aux/sodepends.py -I. -I$(srcdir) -I$(OVS_MANDIR)
$(MAN_ROOTS) >$(@F).tmp
+
@PYTHONPATH=$(OVS_SRCDIR)/python$(psep)$$PYTHONPATH$(psep)$(srcdir)/python
$(PYTHON3) $(srcdir)/build-aux/sodepends.py -I. -I$(srcdir) -I$(OVS_MANDIR)
$(MAN_ROOTS) >$(@F).tmp
@if cmp -s $(@F).tmp $@; then \
touch $@; \
rm -f $(@F).tmp; \
diff --git a/automake.mk b/automake.mk
index ad801f1e5..591e00751 100644
--- a/automake.mk
+++ b/automake.mk
@@ -6,19 +6,17 @@ CLEANFILES += ovn-architecture.7
#
# If "python" or "dot" is not available, then we do not add graphical diagram
# to the documentation.
-if HAVE_PYTHON
if HAVE_DOT
OVSDB_DOT = $(run_python) ${OVSDIR}/ovsdb/ovsdb-dot.in
ovn-nb.gv: ${OVSDIR}/ovsdb/ovsdb-dot.in $(srcdir)/ovn-nb.ovsschema
$(AM_V_GEN)$(OVSDB_DOT) --no-arrows $(srcdir)/ovn-nb.ovsschema > $@
ovn-nb.pic: ovn-nb.gv ${OVSDIR}/ovsdb/dot2pic
- $(AM_V_GEN)(dot -T plain < ovn-nb.gv | $(PYTHON)
${OVSDIR}/ovsdb/dot2pic -f 3) > $@.tmp && \
+ $(AM_V_GEN)(dot -T plain < ovn-nb.gv | $(PYTHON3)
${OVSDIR}/ovsdb/dot2pic -f 3) > $@.tmp && \
mv $@.tmp $@
OVN_NB_PIC = ovn-nb.pic
OVN_NB_DOT_DIAGRAM_ARG = --er-diagram=$(OVN_NB_PIC)
CLEANFILES +=
Re: [ovs-dev] (no subject)
Did you receive our business proposal email ? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 00/20] netdev datapath actions offload
On 20.11.2019 16:28, Eli Britstein wrote: > Currently, netdev datapath offload only accelerates the flow match > sequence by associating a mark per flow. This series introduces the full > offload of netdev datapath flows by having the HW also perform the flow > actions. > > This series adds HW offload for output, drop, set MAC, set IPv4, set > TCP/UDP ports and tunnel push actions using the DPDK rte_flow API. > > Travis passed: https://travis-ci.org/elibritstein/OVS/builds/614511472 > > Eli Britstein (18): > sparse: rte_flow.h: Adapt according to DPDK 18.11.2 > netdev-offload-dpdk: Refactor flow patterns and actions code > netdev-offload-dpdk-flow: Dynamically allocate pattern items > netdev-offload-dpdk: Fix typo > netdev-dpdk: Improve HW offload flow debuggability > netdev-dpdk: Introduce flow_flush method > netdev-offload-dpdk: Introduce flow flush callback > netdev-offload-dpdk: Framework for actions offload > netdev-dpdk: Introduce rte flow query function > netdev-offload-dpdk: Implement flow stats get > dpif-netdev: Populate dpif class field in offload struct > netdev-dpdk: Getter function for dpdk port id API > netdev-offload-dpdk-flow: Support offload of output action > netdev-offload-dpdk-flow: Support offload of drop action > netdev-offload-dpdk-flow: Support offload of set MAC actions > netdev-offload-dpdk-flow: Support offload of set IPv4 actions > netdev-offload-dpdk-flow: Support offload of clone tnl_push/output > actions > netdev-offload-dpdk-flow: Support offload of set TCP/UDP ports actions > > Ophir Munk (2): > netdev: Add netdev function: flow_stats_get() > dpif-netdev: Read hw stats during flow_dump_next() call > > NEWS | 3 + > include/sparse/rte_flow.h | 810 ++--- > lib/automake.mk | 4 +- > lib/dpif-netdev.c | 42 +- > lib/netdev-dpdk.c | 76 > lib/netdev-dpdk.h | 11 + > lib/netdev-offload-dpdk-flow.c| 916 > ++ > lib/netdev-offload-dpdk-private.h | 65 +++ > lib/netdev-offload-dpdk.c | 535 +++--- > lib/netdev-offload-provider.h | 7 + > lib/netdev-offload.c | 12 + > lib/netdev-offload.h | 3 + > 12 files changed, 1969 insertions(+), 515 deletions(-) > create mode 100644 lib/netdev-offload-dpdk-flow.c > create mode 100644 lib/netdev-offload-dpdk-private.h > Hi. Thanks for working on this! I really roughly read the patch-set and made a few comments for the places that caught my eyes. Few general comments for the patches: * I don't understand why all of this refactoring was done. Maybe it's because I didn't try to apply the series and look at it as a complete change, but for now it's unclear for me, especially because we already refactored this code few times before and you're reverting some of these changes. Also the need to have netdev-offload-dpdk-flow.c is not clear. * Commit messages are needed. Please, add some with a brief explanation on what happens and why. * If it's not hard, since you will re-spin the series anyway, please, add some periods to the ends of a subject lines. * You need to also update the 'Flow Hardware Offload' section in the Documentation/howto/dpdk.rst. * And, please, mark these new features as experimental in NEWS. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] Datapath: Change in openvswitch kernel module to support MPLS label depth of 3 in ingress direction.
On 11/21/2019 10:07 PM, Martin Varghese wrote:
From: Martin Varghese
Upstream commit:
commit fbdcdd78da7c95f1b970d371e1b23cbd3aa990f3
Author: Martin Varghese
Date: Mon Nov 4 07:27:44 2019 +0530
Change in Openvswitch to support MPLS label depth of 3 in ingress
direction
The openvswitch was supporting a MPLS label depth of 1 in the
ingress direction though the userspace OVS supports a max depth
of 3 labels.This change enables openvswitch module to support a
max depth of 3 labels in the ingress.
Signed-off-by: Martin Varghese
Acked-by: Pravin B Shelar
Signed-off-by: David S. Miller
Signed-off-by: Martin Varghese
Thanks Martin.
Tested-by: Greg Rose
Reviewed-by: Greg Rose
---
datapath/actions.c | 2 +-
datapath/flow.c | 20
datapath/flow.h | 8 +++--
datapath/flow_netlink.c | 85 -
tests/system-traffic.at | 39 +++
5 files changed, 122 insertions(+), 32 deletions(-)
diff --git a/datapath/actions.c b/datapath/actions.c
index a44e804..fbf4457 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -276,7 +276,7 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key
*flow_key,
}
stack->label_stack_entry = lse;
- flow_key->mpls.top_lse = lse;
+ flow_key->mpls.lse[0] = lse;
return 0;
}
diff --git a/datapath/flow.c b/datapath/flow.c
index 916f7f4..6dc7402 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -659,27 +659,35 @@ static int key_extract_l3l4(struct sk_buff *skb, struct
sw_flow_key *key)
memset(>ipv4, 0, sizeof(key->ipv4));
}
} else if (eth_p_mpls(key->eth.type)) {
- size_t stack_len = MPLS_HLEN;
+ u8 label_count = 1;
+ memset(>mpls, 0, sizeof(key->mpls));
skb_set_inner_network_header(skb, skb->mac_len);
while (1) {
__be32 lse;
- error = check_header(skb, skb->mac_len + stack_len);
+ error = check_header(skb, skb->mac_len +
+label_count * MPLS_HLEN);
if (unlikely(error))
return 0;
memcpy(, skb_inner_network_header(skb), MPLS_HLEN);
- if (stack_len == MPLS_HLEN)
- memcpy(>mpls.top_lse, , MPLS_HLEN);
+ if (label_count <= MPLS_LABEL_DEPTH)
+ memcpy(>mpls.lse[label_count - 1], ,
+ MPLS_HLEN);
- skb_set_inner_network_header(skb, skb->mac_len + stack_len);
+ skb_set_inner_network_header(skb, skb->mac_len +
+label_count * MPLS_HLEN);
if (lse & htonl(MPLS_LS_S_MASK))
break;
- stack_len += MPLS_HLEN;
+ label_count++;
}
+ if (label_count > MPLS_LABEL_DEPTH)
+ label_count = MPLS_LABEL_DEPTH;
+
+ key->mpls.num_labels_mask = GENMASK(label_count - 1, 0);
} else if (key->eth.type == htons(ETH_P_IPV6)) {
int nh_len; /* IPv6 Header + Extensions */
diff --git a/datapath/flow.h b/datapath/flow.h
index 5560300..4ad5363 100644
--- a/datapath/flow.h
+++ b/datapath/flow.h
@@ -43,6 +43,7 @@ enum sw_flow_mac_proto {
MAC_PROTO_ETHERNET,
};
#define SW_FLOW_KEY_INVALID 0x80
+#define MPLS_LABEL_DEPTH 3
/* Store options at the end of the array if they are less than the
* maximum size. This allows us to get the benefits of variable length
@@ -98,9 +99,6 @@ struct sw_flow_key {
*/
union {
struct {
- __be32 top_lse; /* top label stack entry */
- } mpls;
- struct {
u8 proto; /* IP protocol or lower 8 bits of ARP
opcode. */
u8 tos; /* IP ToS. */
u8 ttl; /* IP TTL/hop limit. */
@@ -148,6 +146,10 @@ struct sw_flow_key {
} nd;
};
} ipv6;
+ struct {
+ u32 num_labels_mask;/* labels present bitmap of
effective length MPLS_LABEL_DEPTH */
+ __be32 lse[MPLS_LABEL_DEPTH]; /* label stack entry
*/
+ } mpls;
struct ovs_key_nsh nsh; /* network service header */
};
struct {
diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
index 35f13d7..9fc1a19 100644
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c
@@ -438,7 +438,7 @@ static const struct ovs_len_tbl
ovs_key_lens[OVS_KEY_ATTR_MAX + 1] = {
Re: [ovs-dev] [PATCH v5 ovn 2/4] ovn-controller: Add per node states to I-P engine.
On Wed, Nov 20, 2019 at 9:21 AM Dumitru Ceara wrote:
>
> On Tue, Nov 19, 2019 at 7:16 PM Han Zhou wrote:
> >
> >
> >
> > On Tue, Nov 19, 2019 at 1:58 AM Dumitru Ceara wrote:
> > >
> > > On Tue, Nov 19, 2019 at 1:41 AM Han Zhou wrote:
> > > >
> > > > 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?
> > >
> > > My reasoning was that the engine graph is static (i.e., we build it
> > > once at startup and it never changes afterwards) so all recursion
> > > trees are always identical.
> > >
> > I agree that the graph is static, which makes non-recursive a good option,
> > but it doesn't necessarily mean it simplifies code than the recursive
> > version :)
>
> I might also just be my personal view that debugging recursive
> functions is more complicated than with iterative ones :)
>
> >
> > > Moreover, with adding engine node explicit states we don't really need
> > > to store a run_id in the nodes because we have the state of each node
> > > after an engine run. I think removing the run_id is a good idea
> > > because we minimize the external state the user of the engine should
> > > manage (in this case engine_run_id and it's incrementing logic).
> > >
> > > However, if we keep the engine processing in a recursive fashion then
> > > for each of the recursive operations (engine_run, engine_need_run,
> > > engine_init, engine_cleanup) we need a way to avoid executing the
> > > operation twice for a given node. This can be done by adding more
> > > flags to the nodes (or more state values) but given that our DAG is
> > > fixed, precomputing the processing order made more sense to me. What
> > > do you think?
> > >
> > It is true that run_id can be completely avoided with the non-recursive
> > version. However, it can be kept as internal variable for recursive
> > version, to skip repeated access of a node, if we use the engine_init_run()
> > to increment it internally. I think we don't really need any more flags
> > than the non-recursive version other than this internal run_id, and we can
> > remove the external engine_run_id incrementing logic for recursive version,
> > too.
>
> Yes, with an internal run_id this could work.
>
> >
> > > >
> > > > > - 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)
> > > > >
Re: [ovs-dev] [PATCH 15/20] netdev-offload-dpdk-flow: Support offload of output action
On 20.11.2019 16:28, Eli Britstein wrote:
> Signed-off-by: Eli Britstein
> Reviewed-by: Oz Shlomo
> ---
> NEWS | 2 +
> lib/netdev-offload-dpdk-flow.c | 87
> --
> 2 files changed, 85 insertions(+), 4 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 88b818948..ca9c2b230 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -10,6 +10,8 @@ Post-v2.12.0
> if supported by libbpf.
> * Add option to enable, disable and query TCP sequence checking in
> conntrack.
> + - DPDK:
> + * Add hardware offload support for output actions.
>
> v2.12.0 - 03 Sep 2019
> -
> diff --git a/lib/netdev-offload-dpdk-flow.c b/lib/netdev-offload-dpdk-flow.c
> index dbbc45e99..6e7efb315 100644
> --- a/lib/netdev-offload-dpdk-flow.c
> +++ b/lib/netdev-offload-dpdk-flow.c
> @@ -274,6 +274,28 @@ ds_put_flow_action(struct ds *s, const struct
> rte_flow_action *actions)
> } else {
> ds_put_cstr(s, " RSS = null\n");
> }
> +} else if (actions->type == RTE_FLOW_ACTION_TYPE_COUNT) {
> +const struct rte_flow_action_count *count = actions->conf;
> +
> +ds_put_cstr(s, "rte flow count action:\n");
> +if (count) {
> +ds_put_format(s,
> + " Count: shared=%d, id=%d\n",
> + count->shared, count->id);
> +} else {
> +ds_put_cstr(s, " Count = null\n");
> +}
> +} else if (actions->type == RTE_FLOW_ACTION_TYPE_PORT_ID) {
> +const struct rte_flow_action_port_id *port_id = actions->conf;
> +
> +ds_put_cstr(s, "rte flow port-id action:\n");
> +if (port_id) {
> +ds_put_format(s,
> + " Port-id: original=%d, id=%d\n",
> + port_id->original, port_id->id);
> +} else {
> +ds_put_cstr(s, " Port-id = null\n");
> +}
> } else {
> ds_put_format(s, "unknown rte flow action (%d)\n", actions->type);
> }
> @@ -531,19 +553,76 @@ netdev_dpdk_flow_patterns_add(struct flow_patterns
> *patterns,
> return 0;
> }
>
> +static void
> +netdev_dpdk_flow_add_count_action(struct flow_actions *actions)
> +{
> +struct rte_flow_action_count *count = xzalloc(sizeof *count);
> +
> +count->shared = 0;
> +/* Each flow has a single count action, so no need of id */
> +count->id = 0;
> +add_flow_action(actions, RTE_FLOW_ACTION_TYPE_COUNT, count);
> +}
> +
> +static void
> +netdev_dpdk_flow_add_port_id_action(struct flow_actions *actions,
> +struct netdev *outdev)
> +{
> +struct rte_flow_action_port_id *port_id = xzalloc(sizeof *port_id);
> +
> +port_id->id = netdev_dpdk_get_port_id(outdev);
> +add_flow_action(actions, RTE_FLOW_ACTION_TYPE_PORT_ID, port_id);
> +}
> +
> +static int
> +netdev_dpdk_flow_add_output_action(struct flow_actions *actions,
> + const struct nlattr *nla,
> + struct offload_info *info)
> +{
> +struct netdev *outdev;
> +odp_port_t port;
> +
> +port = nl_attr_get_odp_port(nla);
> +outdev = netdev_ports_get(port, info->dpif_class);
> +if (outdev == NULL) {
> +VLOG_DBG_RL(_rl,
> +"Cannot find netdev for odp port %d", port);
> +return -1;
> +}
> +if (!netdev_dpdk_flow_api_supported(outdev)) {
This doesn't look correct. I mean, maybe we need to check if port is
really the port on a same piece of hardware. Will the flow setup fail
in this case? Will code fallback to the MARK+RSS?
> +VLOG_DBG_RL(_rl,
> +"Output to %s cannot be offloaded",
> +netdev_get_name(outdev));
> +return -1;
> +}
> +
> +netdev_dpdk_flow_add_count_action(actions);
> +netdev_dpdk_flow_add_port_id_action(actions, outdev);
> +netdev_close(outdev);
> +
> +return 0;
> +}
> +
> int
> netdev_dpdk_flow_actions_add(struct flow_actions *actions,
> struct nlattr *nl_actions,
> size_t nl_actions_len,
> - struct offload_info *info OVS_UNUSED)
> + struct offload_info *info)
> {
> struct nlattr *nla;
> size_t left;
>
> NL_ATTR_FOR_EACH_UNSAFE (nla, left, nl_actions, nl_actions_len) {
> -VLOG_DBG_RL(_rl,
> -"Unsupported action type %d", nl_attr_type(nla));
> -return -1;
> +if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT &&
> +left <= NLA_ALIGN(nla->nla_len)) {
> +if (netdev_dpdk_flow_add_output_action(actions, nla, info )) {
> +return -1;
> +}
> +} else {
> +VLOG_DBG_RL(_rl,
> +"Unsupported action type %d", nl_attr_type(nla));
> +return -1;
> +}
> }
>
[ovs-dev] [PATCH v6 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 | 32 +-
lib/inc-proc-eng.h | 28 +
3 files changed, 169 insertions(+), 121 deletions(-)
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 201ef9e..f6945fb 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 v6 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 a588531..201ef9e 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 v6 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.
- turn the global 'engine_abort_recompute' into an argument to be passed to
engine_run(). It's relevant only in the current run context anyway as
we reset it before every call to engine_run().
Signed-off-by: Dumitru Ceara
---
controller/ovn-controller.c | 84 ---
lib/inc-proc-eng.c | 242 ---
lib/inc-proc-eng.h | 74 +
3 files changed, 276 insertions(+), 124 deletions(-)
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index c56190f..a588531 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)
[ovs-dev] [PATCH v6 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 | 414 ---
lib/inc-proc-eng.c | 338 +++
lib/inc-proc-eng.h | 103 ---
3 files changed, 570 insertions(+), 285 deletions(-)
---
v6:
- Address Han's comments:
- Call engine_recompute only once for a node if at least one of its input
nodes' change handler returns false.
- Simplify the incremental engine API and internally store the
topologically sorted engine nodes.
- Change 'engine_abort_recompute' from global variable to argument to be
passed to engine_run(). It's only relevant in one run context anyway
as we used to reset it before every call to engine_run().
- engine_init_run() and engine_has_run() now check all the nodes in the
engine instead of a single one.
- Change 'engine_node_valid()' to call the node's 'is_valid' method only
if the node is not in state EN_UPDATED or EN_VALID.
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] [PATCH v6 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 | 120 +--
lib/inc-proc-eng.h |7 ++-
3 files changed, 103 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..ff07ad9 100644
--- a/lib/inc-proc-eng.c
+++ b/lib/inc-proc-eng.c
@@ -129,14 +129,68 @@ 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, false otherwise. */
Re: [ovs-dev] [PATCH 12/20] dpif-netdev: Read hw stats during flow_dump_next() call
On 20.11.2019 16:28, Eli Britstein wrote:
> From: Ophir Munk
>
> Flow stats are retrieved by revalidator threads. Specifically for
> dpif-netdev the function dpif_netdev_flow_dump_next() is called.
> When the flow is fully offloaded reading the PMD SW stats alone will
> result in no updates and will cause the revalidator to falsely delete
> the flow after some aging time.
> This commit adds a new function called dp_netdev_offload_used() which
> reads the hw counters during the flow_dump_next() call.
> The new function calls netdev_flow_stats_get() which in turn reads the
> hardware stats by calling DPDK rte_flow_query() API.
>
> Signed-off-by: Ophir Munk
> Reviewed-by: Oz Shlomo
> Signed-off-by: Eli Britstein
> ---
> lib/dpif-netdev.c | 40 +++-
> 1 file changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 4720ba1ab..d9f28cfcd 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -527,6 +527,7 @@ struct dp_netdev_flow {
>
> bool dead;
> uint32_t mark; /* Unique flow mark assigned to a flow */
> +bool actions_offloaded; /* true if flow is fully offloaded */
>
> /* Statistics. */
> struct dp_netdev_flow_stats stats;
> @@ -2422,6 +2423,7 @@ dp_netdev_flow_offload_put(struct dp_flow_offload_item
> *offload)
>offload->actions_len, >mega_ufid, ,
>NULL);
> ovs_mutex_unlock(>dp->port_mutex);
> +flow->actions_offloaded = info.actions_offloaded;
>
> if (ret) {
> goto err_free;
> @@ -3073,7 +3075,7 @@ dp_netdev_flow_to_dpif_flow(const struct dp_netdev_flow
> *netdev_flow,
> flow->pmd_id = netdev_flow->pmd_id;
> get_dpif_flow_stats(netdev_flow, >stats);
>
> -flow->attrs.offloaded = false;
> +flow->attrs.offloaded = netdev_flow->actions_offloaded;
> flow->attrs.dp_layer = "ovs";
We, probably, should change the 'dp_layer' for fully offloaded flows.
> }
>
> @@ -3244,6 +3246,7 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
> flow->dead = false;
> flow->batch = NULL;
> flow->mark = INVALID_FLOW_MARK;
> +flow->actions_offloaded = false;
> *CONST_CAST(unsigned *, >pmd_id) = pmd->core_id;
> *CONST_CAST(struct flow *, >flow) = match->flow;
> *CONST_CAST(ovs_u128 *, >ufid) = *ufid;
> @@ -3598,6 +3601,37 @@ dpif_netdev_flow_dump_thread_destroy(struct
> dpif_flow_dump_thread *thread_)
> free(thread);
> }
>
> +static int
> +dpif_netdev_offload_used(struct dp_netdev_flow *netdev_flow,
> + struct dp_netdev_pmd_thread *pmd)
> +{
> +int ret;
> +struct dp_netdev_port *port;
> +struct dpif_flow_stats stats;
> +
> +odp_port_t in_port = netdev_flow->flow.in_port.odp_port;
> +
> +ovs_mutex_lock(>dp->port_mutex);
> +port = dp_netdev_lookup_port(pmd->dp, in_port);
Use netdev_ports_get() instead.
See: https://patchwork.ozlabs.org/patch/1199525/
> +if (!port) {
> +ovs_mutex_unlock(>dp->port_mutex);
> +return -1;
> +}
> +/* get offloaded stats */
> +ret = netdev_flow_stats_get(port->netdev, _flow->mega_ufid,
> );
> +ovs_mutex_unlock(>dp->port_mutex);
> +if (ret) {
> +return -1;
> +}
> +if (stats.n_packets) {
> +atomic_store_relaxed(_flow->stats.used, pmd->ctx.now / 1000);
> +non_atomic_ullong_add(_flow->stats.packet_count,
> stats.n_packets);
> +non_atomic_ullong_add(_flow->stats.byte_count, stats.n_bytes);
> +}
> +
> +return 0;
> +}
> +
> static int
> dpif_netdev_flow_dump_next(struct dpif_flow_dump_thread *thread_,
> struct dpif_flow *flows, int max_flows)
> @@ -3638,6 +3672,10 @@ dpif_netdev_flow_dump_next(struct
> dpif_flow_dump_thread *thread_,
> netdev_flows[n_flows] = CONTAINER_OF(node,
> struct dp_netdev_flow,
> node);
> +/* Read hardware stats in case of hardware offload */
> +if (netdev_flows[n_flows]->actions_offloaded) {
> +dpif_netdev_offload_used(netdev_flows[n_flows], pmd);
> +}
> }
> /* When finishing dumping the current pmd thread, moves to
> * the next. */
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 07/20] netdev-offload-dpdk: Introduce flow flush callback
On 20.11.2019 16:28, Eli Britstein wrote:
> Introduce flow flush callback for dpdk offloaded flows.
>
> Signed-off-by: Eli Britstein
> Reviewed-by: Oz Shlomo
> ---
> lib/netdev-offload-dpdk.c | 16
> 1 file changed, 16 insertions(+)
>
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> index 6e1ca8a0d..64873759d 100644
> --- a/lib/netdev-offload-dpdk.c
> +++ b/lib/netdev-offload-dpdk.c
> @@ -307,6 +307,21 @@ netdev_offload_dpdk_flow_del(struct netdev *netdev,
> const ovs_u128 *ufid,
> return netdev_offload_dpdk_destroy_flow(netdev, ufid, rte_flow);
> }
>
> +static int
> +netdev_offload_dpdk_flow_flush(struct netdev *netdev)
> +{
> +struct rte_flow_error error;
> +int ret;
> +
> +ret = netdev_dpdk_rte_flow_flush(netdev, );
I don't think that it's enough to only flush flows from the device,
you need to destroy all the structures allocated for already offloaded
flows in OVS, i.e. at least call ufid_to_rte_flow_disassociate()
for each of them.
> +if (ret) {
> +VLOG_ERR("%s: rte flow flush error: %u : message : %s\n",
> + netdev_get_name(netdev), error.type, error.message);
> +}
> +
> +return ret;
> +}
> +
> static int
> netdev_offload_dpdk_init_flow_api(struct netdev *netdev)
> {
> @@ -315,6 +330,7 @@ netdev_offload_dpdk_init_flow_api(struct netdev *netdev)
>
> const struct netdev_flow_api netdev_offload_dpdk = {
> .type = "dpdk_flow_api",
> +.flow_flush = netdev_offload_dpdk_flow_flush,
> .flow_put = netdev_offload_dpdk_flow_put,
> .flow_del = netdev_offload_dpdk_flow_del,
> .init_flow_api = netdev_offload_dpdk_init_flow_api,
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2 2/2] northd: Improve handling of pause and resume
Ok, this time I am at a loss of what's wrong with the submission, I would kindly request reviews of the submission as-is while I figure out my grievances with the bot. -- Frode Nordahl On Fri, Nov 22, 2019 at 4:03 PM 0-day Robot wrote: > > Bleep bloop. Greetings Frode Nordahl, I am a robot and I have tried out your > patch. > Thanks for your contribution. > > I encountered some error that I wasn't expecting. See the details below. > > > git-am: > fatal: sha1 information is lacking or useless (tests/ovn-northd.at). > Repository lacks necessary blobs to fall back on 3-way merge. > Cannot fall back to three-way merge. > Patch failed at 0001 northd: Improve handling of pause and resume > 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
Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.
On 22.11.2019 14:51, Ophir Munk wrote:
> Hi Ilya,
> Thanks for your explanations.
> We are using just one vport ("vxlan0") and still the netdev flow
> api pointer is NULL during a flow_put() call.
> Following is a root cause description.
>
> In short:
> 1. A new port->netdev named "vxlan_sys_4789" is created as part of adding the
> "vxlan0" netdev port.
> The new port->netdev is created without initializing it with flow api
> pointers.
> 2. We call netdev_init_flow_api(netdev) on the "vxlan0" netdev.
> 3. During flow_put we call with the port->netdev "vxlan_sys_4789", whose flow
> api pointers are NULL.
>
> In details:
> 1. For "vxlan0" interface we have a call dpif_port_add (dpif.c)
> 2. Inside this function we have a call
> dpif->dpif_class->port_add(dpif, netdev, _no);
> Which calls the netdev API dpif_netdev_port_add()
> 3. Inside dpif_netdev_port_add() dpif_port gets a new name: "vxlan_sys_4789":
> dpif_port = netdev_vport_get_dpif_port(netdev, namebuf, sizeof namebuf);
>
> Then we call do_add_port(dp, dpif_port, netdev_get_type(netdev), port_no) with
> the new name.
> We have a sequence of calls that eventually creates a new netdev for
> dpif_port "vxlan_sys_4789"
> do_add_port(dp, dpif_port, netdev_get_type(netdev), port_no);
>==> port_create(devname, type, port_no, );
>==> netdev_open(devname, type, );
> ==> netdev = rc->class->alloc();
> The newly created port holds the new port->netdev and it is added to hash map:
> hmap_insert(>ports, >node, hash_port_no(port_no));
> Please note port->netdev was not initialized with flow api.
>
> Back to dpif_port_add (dpif.c) we then have a call at the end to
> netdev_ports_insert() (file netdev_offload.c) which calls
> netdev_init_flow_api("vxlan0" netdev).
> The netdev parameter is the "vxlan0" netdev and not the port->netdev
> "vxlan_sys_4789"
> that was created.
>
> When dp_netdev_flow_add() it called (file dpif-netdev.c) we will find
> the port->netdev "vxlan_sys_4789".
> As mentioned this netdev has never been initialized with flow API.
> Therefore its flow pointers are NULL.
> The pointers were set successfully on the "vxlan0" netdev - but these pointer
> are not
> used during a flow_put() call.
>
> By the way for a "dpdk" type netdev this issue does not happen. It is because
> the dpif port
> name has the same name as the interface name.
> No new netdev is allocated. Therefore flow_put will call with the
> port->netdev that was
> initialized with flow api pointers.
> Hence there is no issue in this case.
>
> I would appreciate your comments on this.
>
Thanks for figuring this out!
Yes, it seems a bit strange that dpif-netdev creates new netdev instance
and maybe we could avoid that somehow. However, this unveiled the bug
which is improper usage of offloading API. Datapath should request
netdevs from the netdev-offload module with netdev_ports_get() and use
resulted netdev for netdev_flow_put(), but dpif-netdev for some reason
performs lookup in the datapath port list and that is the issue.
I prepared the patch for that:
https://patchwork.ozlabs.org/patch/1199525/
Some changes will be needed to use it along with my previous patch-set
for vport offloading, but they are straightforward.
Best regards, Ilya Maximets.
> Regards,
> Ophir
>
>> -Original Message-
>> From: Ilya Maximets
>> Sent: Wednesday, November 20, 2019 7:00 PM
>> To: Ophir Munk ; Ilya Maximets
>> ; ovs-dev@openvswitch.org
>> Cc: Ameer Mahagneh ; Roni Bar Yanai
>> ; Eveline Raine ; Oz
>> Shlomo ; Eli Britstein
>> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
>>
>> On 20.11.2019 17:52, Ophir Munk wrote:
>>> Hi Ilya,
>>> I think the problem is that the newly created netdev called
>> "vxlan_sys_4789" never goes through a netdev_init_flow_api(netdev); call.
>>
>> 'vxlan_sys_4789' is not a netdev. It's a dpif_port.
>> There is a corresponding 'netdev', but only 'ofproto' knows which one.
>>
>>> Where do you suggest adding this call?
>>>
>>> Regards,
>>> Ophir
>>>
-Original Message-
From: Ilya Maximets
Sent: Tuesday, November 19, 2019 8:30 PM
To: Ophir Munk ; Ilya Maximets
; ovs-dev@openvswitch.org
Cc: Ameer Mahagneh ; Roni Bar Yanai
; Eveline Raine ; Oz
Shlomo ; Eli Britstein
Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
On 19.11.2019 18:20, Ophir Munk wrote:
> Hi Ilya,
> Thanks for the patch set which adds the dpif hint inside the netdev
>> struct.
It is really helpful.
> Our goal is to have flow_put() calls on vxlan devices, using the
> existing dpdk
flow API.
> We added logic inside netdev_dpdk_flow_api_supported() to accept
> vxlan devices and indeed we see in the log:
> "netdev_offload|INFO|vxlan0: Assigned flow API 'dpdk_flow_api'".
> However, there is no flow_put() on the vxlan device.
> We see our own printouts in dpif-netdev such as:
>
[ovs-dev] [PATCH] dpif-netdev: Use netdev-offload API for port lookup while offloading.
Currently, while offloading, userspace datapath tries to lookup netdev
in a local port list of the datapath interface instance. However,
there is no guarantee that these netdevs are the same netdevs that
netdev-offload module operates with and, as a result, there is no any
guarantee that these netdev instances has initialized flow API.
dpif-netdev should request ports from the netdev-offload module as
intended by flow offloading API in a same way as dpif-netlink does.
This will also give us performance benefits because we don't need to
hold global port mutex anymore.
We're not noticing any significant issues with current code, but
it will become a serious issue in the future, e.g. with offloading
for virtual tunneling ports.
Reported-by: Ophir Munk
Fixes: 241bad15d99a ("dpif-netdev: associate flow with a mark id")
Signed-off-by: Ilya Maximets
---
lib/dpif-netdev.c | 22 ++
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 5142bad1d..885a4df36 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -2266,15 +2266,14 @@ mark_to_flow_disassociate(struct dp_netdev_pmd_thread
*pmd,
* remove the flow from hardware and free the mark.
*/
if (flow_mark_has_no_ref(mark)) {
-struct dp_netdev_port *port;
+struct netdev *port;
odp_port_t in_port = flow->flow.in_port.odp_port;
-ovs_mutex_lock(>dp->port_mutex);
-port = dp_netdev_lookup_port(pmd->dp, in_port);
+port = netdev_ports_get(in_port, pmd->dp->dpif->dpif_class);
if (port) {
-ret = netdev_flow_del(port->netdev, >mega_ufid, NULL);
+ret = netdev_flow_del(port, >mega_ufid, NULL);
+netdev_close(port);
}
-ovs_mutex_unlock(>dp->port_mutex);
flow_mark_free(mark);
VLOG_DBG("Freed flow mark %u\n", mark);
@@ -2372,12 +2371,12 @@ dp_netdev_flow_offload_del(struct dp_flow_offload_item
*offload)
static int
dp_netdev_flow_offload_put(struct dp_flow_offload_item *offload)
{
-struct dp_netdev_port *port;
struct dp_netdev_pmd_thread *pmd = offload->pmd;
struct dp_netdev_flow *flow = offload->flow;
odp_port_t in_port = flow->flow.in_port.odp_port;
bool modification = offload->op == DP_NETDEV_FLOW_OFFLOAD_OP_MOD;
struct offload_info info;
+struct netdev *port;
uint32_t mark;
int ret;
@@ -2411,17 +2410,16 @@ dp_netdev_flow_offload_put(struct dp_flow_offload_item
*offload)
}
info.flow_mark = mark;
-ovs_mutex_lock(>dp->port_mutex);
-port = dp_netdev_lookup_port(pmd->dp, in_port);
-if (!port || netdev_vport_is_vport_class(port->netdev->netdev_class)) {
-ovs_mutex_unlock(>dp->port_mutex);
+port = netdev_ports_get(in_port, pmd->dp->dpif->dpif_class);
+if (!port || netdev_vport_is_vport_class(port->netdev_class)) {
+netdev_close(port);
goto err_free;
}
-ret = netdev_flow_put(port->netdev, >match,
+ret = netdev_flow_put(port, >match,
CONST_CAST(struct nlattr *, offload->actions),
offload->actions_len, >mega_ufid, ,
NULL);
-ovs_mutex_unlock(>dp->port_mutex);
+netdev_close(port);
if (ret) {
goto err_free;
--
2.17.1
___
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 Mon, Nov 18, 2019 at 5:20 PM Ben Pfaff wrote:
>
> 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?
>
Hi Ben,
yes, my situation was a bit artificious, but you can get a similar
situation in practice.
Imagine a router with some subnetworks behind it on N levels, with
some nodes hosting virtual machines.
Windows and Linux have different default TTL values, 128 and 64
respectively, so you could see N*2 different TTL values.
Bye,
--
Matteo Croce
per aspera ad upstream
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2 2/2] northd: Improve handling of pause and resume
Bleep bloop. Greetings Frode Nordahl, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: fatal: sha1 information is lacking or useless (tests/ovn-northd.at). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 northd: Improve handling of pause and resume 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 ovn v2 2/2] northd: Improve handling of pause and resume
Move paused state to ``struct northd_context`` and pass the
context to paused and status command handlers.
On pause release the OVSDB lock on SB DB.
Re-instante the lock on resume.
Status command will now provide accurate information for 'paused',
'active' and 'standby' states.
Merge separate status command test into the pause and resume tests.
Signed-off-by: Frode Nordahl
---
northd/ovn-northd.8.xml | 9 +++--
northd/ovn-northd.c | 87 +++--
tests/ovn-northd.at | 24 +++-
3 files changed, 79 insertions(+), 41 deletions(-)
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 9734e79e6..c6d5d96b9 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -74,13 +74,15 @@
pause
Pauses the ovn-northd operation from processing any Northbound and
-Southbound database changes.
+Southbound database changes. This will also instruct ovn-northd to
+drop any lock on SB DB.
resume
Resumes the ovn-northd operation to process Northbound and
-Southbound database contents and generate logical flows.
+Southbound database contents and generate logical flows. This will
+also instruct ovn-northd to aspire for the lock on SB DB.
is-paused
@@ -91,7 +93,8 @@
status
Prints this server's status. Status will be "active" if ovn-northd has
-acquired OVSDB lock on NB DB, "standby" otherwise.
+acquired OVSDB lock on SB DB, "standby" if it has not or "paused" if
+this instance is paused.
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index a943e1037..e19515d14 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -65,6 +65,7 @@ struct northd_context {
struct ovsdb_idl_index *sbrec_ha_chassis_grp_by_name;
struct ovsdb_idl_index *sbrec_mcast_group_by_name_dp;
struct ovsdb_idl_index *sbrec_ip_mcast_by_dp;
+bool paused;
};
/* An IPv4 or IPv6 address */
@@ -10836,6 +10837,8 @@ add_column_noalert(struct ovsdb_idl *idl,
ovsdb_idl_omit_alert(idl, column);
}
+#define OVN_NORTHD_SB_DB_LOCK_NAME "ovn_northd"
+
int
main(int argc, char *argv[])
{
@@ -10843,8 +10846,10 @@ main(int argc, char *argv[])
struct unixctl_server *unixctl;
int retval;
bool exiting;
-bool paused;
bool had_lock;
+struct northd_context ctx;
+
+memset(, 0, sizeof(ctx));
fatal_ignore_sigpipe();
ovs_cmdl_proctitle_init(argc, argv);
@@ -10866,11 +10871,11 @@ main(int argc, char *argv[])
exit(EXIT_FAILURE);
}
unixctl_command_register("exit", "", 0, 0, ovn_northd_exit, );
-unixctl_command_register("pause", "", 0, 0, ovn_northd_pause, );
-unixctl_command_register("resume", "", 0, 0, ovn_northd_resume, );
+unixctl_command_register("pause", "", 0, 0, ovn_northd_pause, );
+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);
+ );
+unixctl_command_register("status", "", 0, 0, ovn_northd_status, );
daemonize_complete();
@@ -11075,23 +11080,21 @@ main(int argc, char *argv[])
/* Ensure that only a single ovn-northd is active in the deployment by
* 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");
+ovsdb_idl_set_lock(ovnsb_idl_loop.idl, OVN_NORTHD_SB_DB_LOCK_NAME);
/* Main loop. */
exiting = false;
-paused = false;
+ctx.paused = false;
had_lock = false;
while (!exiting) {
-if (!paused) {
-struct northd_context ctx = {
-.ovnnb_idl = ovnnb_idl_loop.idl,
-.ovnnb_txn = ovsdb_idl_loop_run(_idl_loop),
-.ovnsb_idl = ovnsb_idl_loop.idl,
-.ovnsb_txn = ovsdb_idl_loop_run(_idl_loop),
-.sbrec_ha_chassis_grp_by_name = sbrec_ha_chassis_grp_by_name,
-.sbrec_mcast_group_by_name_dp = sbrec_mcast_group_by_name_dp,
-.sbrec_ip_mcast_by_dp = sbrec_ip_mcast_by_dp,
-};
+if (!ctx.paused) {
+ctx.ovnnb_idl = ovnnb_idl_loop.idl;
+ctx.ovnnb_txn = ovsdb_idl_loop_run(_idl_loop);
+ctx.ovnsb_idl = ovnsb_idl_loop.idl;
+ctx.ovnsb_txn = ovsdb_idl_loop_run(_idl_loop);
+ctx.sbrec_ha_chassis_grp_by_name = sbrec_ha_chassis_grp_by_name;
+ctx.sbrec_mcast_group_by_name_dp = sbrec_mcast_group_by_name_dp;
+ctx.sbrec_ip_mcast_by_dp = sbrec_ip_mcast_by_dp;
if (!had_lock && ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) {
VLOG_INFO("ovn-northd lock
[ovs-dev] [PATCH ovn v2 1/2] northd: Amend ovn-northd pause resume test
The current pause and resume tests rely on that ``ovn-northd`` will hold on to the lock while paused. Update the test to pause both ``ovn-northd`` instances so we may change this behaviour. Replace references to ``ovs-appctl`` with ``ovn-appctl``. Signed-off-by: Frode Nordahl --- tests/ovn-northd.at | 13 ++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index c73fd9003..1c94f2f65 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -912,7 +912,9 @@ AT_SETUP([ovn -- ovn-northd pause and resume]) AT_SKIP_IF([test $HAVE_PYTHON = no]) ovn_start -AT_CHECK([test xfalse = x`as northd ovs-appctl -t ovn-northd is-paused`]) +AT_CHECK([test xfalse = x`as northd ovn-appctl -t ovn-northd is-paused`]) +AT_CHECK([test xfalse = x`as northd-backup ovn-appctl -t ovn-northd \ +is-paused`]) ovn-nbctl ls-add sw0 @@ -927,7 +929,9 @@ OVS_WAIT_UNTIL([ # Now pause the ovn-northd as northd ovs-appctl -t ovn-northd pause -AT_CHECK([test xtrue = x`as northd ovs-appctl -t ovn-northd is-paused`]) +as northd-backup ovs-appctl -t ovn-northd pause +AT_CHECK([test xtrue = x`as northd ovn-appctl -t ovn-northd is-paused`]) +AT_CHECK([test xtrue = x`as northd-backup ovn-appctl -t ovn-northd is-paused`]) ovn-nbctl ls-add sw0 @@ -938,7 +942,10 @@ OVS_WAIT_UNTIL([ # Now resume ovn-northd as northd ovs-appctl -t ovn-northd resume -AT_CHECK([test xfalse = x`as northd ovs-appctl -t ovn-northd is-paused`]) +as northd-backup ovs-appctl -t ovn-northd resume +AT_CHECK([test xfalse = x`as northd ovn-appctl -t ovn-northd is-paused`]) +AT_CHECK([test xfalse = x`as northd-backup ovn-appctl -t ovn-northd \ +is-paused`]) OVS_WAIT_UNTIL([ ovn-sbctl lflow-list sw0 -- 2.24.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 2/2] northd: Improve handling of pause and resume
Frode Nordahl writes: > On Fri, Nov 22, 2019 at 11:46 AM Frode Nordahl > wrote: >> >> On Fri, Nov 22, 2019 at 11:12 AM Frode Nordahl >> wrote: >> > >> > This is weird, the patches apply on an up-to-date ovn repo with git am >> > on blobs from the list. >> > >> > I noticed the mailing list thread order is in reverse with patch >> > number two preceding patch number one, could that be the issue? >> >> It may also be me editing the output from ``git format-patch`` prior to >> submitting, but cannot reproduce the error reported by the bot locally >> with blobs gleaned from list, so would love to hear your views on >> what's going on here. > > Ah yes that must be it, the commit msg is part of the commit sha1 > and apparently I have tampered with it, so this is my bad. > > Sorry about the noise, I'll submit an untampered v2. Glad it worked out :) In the future, you can try and grab the patch from the patchwork instance (https://patchwork.ozlabs.org/project/openvswitch/) and check that it matches and applies. I'll try to improve the output of the bot as well to include base commit information (so at least we can try to reproduce the errors). ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v2 0/2] northd: Improve pause, resume and status
At present when the ``pause`` command is issued any lock the ``ovn-northd`` process holds on the SB DB will be kept. To be able to change this behaviour the tests surrounding pause and resume must be changed, thus I provide the proposed change as a series to demonstrate that the changed tests work both for the current code and the proposed code. Travis passed: https://travis-ci.org/fnordahl/ovn/builds/615446047 Frode Nordahl (2): northd: Amend ovn-northd pause resume test northd: Improve handling of pause and resume northd/ovn-northd.8.xml | 9 +++-- northd/ovn-northd.c | 87 +++-- tests/ovn-northd.at | 35 +++-- 3 files changed, 88 insertions(+), 43 deletions(-) -- 2.24.0 , ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.
Hi Ilya,
Thanks for your explanations.
We are using just one vport ("vxlan0") and still the netdev flow
api pointer is NULL during a flow_put() call.
Following is a root cause description.
In short:
1. A new port->netdev named "vxlan_sys_4789" is created as part of adding the
"vxlan0" netdev port.
The new port->netdev is created without initializing it with flow api pointers.
2. We call netdev_init_flow_api(netdev) on the "vxlan0" netdev.
3. During flow_put we call with the port->netdev "vxlan_sys_4789", whose flow
api pointers are NULL.
In details:
1. For "vxlan0" interface we have a call dpif_port_add (dpif.c)
2. Inside this function we have a call
dpif->dpif_class->port_add(dpif, netdev, _no);
Which calls the netdev API dpif_netdev_port_add()
3. Inside dpif_netdev_port_add() dpif_port gets a new name: "vxlan_sys_4789":
dpif_port = netdev_vport_get_dpif_port(netdev, namebuf, sizeof namebuf);
Then we call do_add_port(dp, dpif_port, netdev_get_type(netdev), port_no) with
the new name.
We have a sequence of calls that eventually creates a new netdev for
dpif_port "vxlan_sys_4789"
do_add_port(dp, dpif_port, netdev_get_type(netdev), port_no);
==> port_create(devname, type, port_no, );
==> netdev_open(devname, type, );
==> netdev = rc->class->alloc();
The newly created port holds the new port->netdev and it is added to hash map:
hmap_insert(>ports, >node, hash_port_no(port_no));
Please note port->netdev was not initialized with flow api.
Back to dpif_port_add (dpif.c) we then have a call at the end to
netdev_ports_insert() (file netdev_offload.c) which calls
netdev_init_flow_api("vxlan0" netdev).
The netdev parameter is the "vxlan0" netdev and not the port->netdev
"vxlan_sys_4789"
that was created.
When dp_netdev_flow_add() it called (file dpif-netdev.c) we will find
the port->netdev "vxlan_sys_4789".
As mentioned this netdev has never been initialized with flow API.
Therefore its flow pointers are NULL.
The pointers were set successfully on the "vxlan0" netdev - but these pointer
are not
used during a flow_put() call.
By the way for a "dpdk" type netdev this issue does not happen. It is because
the dpif port
name has the same name as the interface name.
No new netdev is allocated. Therefore flow_put will call with the port->netdev
that was
initialized with flow api pointers.
Hence there is no issue in this case.
I would appreciate your comments on this.
Regards,
Ophir
> -Original Message-
> From: Ilya Maximets
> Sent: Wednesday, November 20, 2019 7:00 PM
> To: Ophir Munk ; Ilya Maximets
> ; ovs-dev@openvswitch.org
> Cc: Ameer Mahagneh ; Roni Bar Yanai
> ; Eveline Raine ; Oz
> Shlomo ; Eli Britstein
> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
>
> On 20.11.2019 17:52, Ophir Munk wrote:
> > Hi Ilya,
> > I think the problem is that the newly created netdev called
> "vxlan_sys_4789" never goes through a netdev_init_flow_api(netdev); call.
>
> 'vxlan_sys_4789' is not a netdev. It's a dpif_port.
> There is a corresponding 'netdev', but only 'ofproto' knows which one.
>
> > Where do you suggest adding this call?
> >
> > Regards,
> > Ophir
> >
> >> -Original Message-
> >> From: Ilya Maximets
> >> Sent: Tuesday, November 19, 2019 8:30 PM
> >> To: Ophir Munk ; Ilya Maximets
> >> ; ovs-dev@openvswitch.org
> >> Cc: Ameer Mahagneh ; Roni Bar Yanai
> >> ; Eveline Raine ; Oz
> >> Shlomo ; Eli Britstein
> >> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
> >>
> >> On 19.11.2019 18:20, Ophir Munk wrote:
> >>> Hi Ilya,
> >>> Thanks for the patch set which adds the dpif hint inside the netdev
> struct.
> >> It is really helpful.
> >>> Our goal is to have flow_put() calls on vxlan devices, using the
> >>> existing dpdk
> >> flow API.
> >>> We added logic inside netdev_dpdk_flow_api_supported() to accept
> >>> vxlan devices and indeed we see in the log:
> >>> "netdev_offload|INFO|vxlan0: Assigned flow API 'dpdk_flow_api'".
> >>> However, there is no flow_put() on the vxlan device.
> >>> We see our own printouts in dpif-netdev such as:
> >>> "dpif_netdev(dp_netdev_flow_5)|ERR|vxlan_sys_4789: calling netdev
> >> flow_put()"
> >>> but inside netdev_flow_put the flow_api poiner is NULL.
> >>> Any ideas why?
> >>
> >> How many tunneling ports you have?
> >> The case here is that dpif creates only one dpif_port for all the
> >> tunnels with similar config. So, only one of these netdevs will have
> >> flow api initialized and you need to be sure that you're using the
> >> right one. You may find it by the datapath 'port_no' with
> netdev_ports_get().
> >> Just guessing, but this is the one possible reason.
> >>
> >>>
> -Original Message-
> From: Ilya Maximets
> Sent: Friday, November 15, 2019 10:26 PM
> To: Ophir Munk ; Ilya Maximets
> ; ovs-dev@openvswitch.org
> Cc: Ameer Mahagneh ; Roni Bar Yanai
> ; Eveline Raine ; Oz
> Shlomo ; Eli Britstein
>
[ovs-dev] [PATCH ovn] ovn-controller: Consider non-virtual ports first when updating bindings.
There's no guarantee SBREC_PORT_BINDING_TABLE_FOR_EACH will first
return the non-virtual ports and then virtual ports. In the case when a
virtual port is processed before its virtual_parent,
consider_local_datapath might not release it in the current
ovn-controller iteration even though the virtual_parent gets released.
Right now this doesn't trigger any functionality issue because releasing
the virtual_parent triggers a change in the SB DB which will wake up
ovn-controller and trigger a new computation which will also update the
virtual port.
However, this is suboptimal, and we can notice that often ovn-controller
gets the SB update notification before the "transaction successful"
notification. In such cases the incremental engine doesn't run
(ovnsb_idl_txn == NULL) and a full recompute is scheduled for the next
run. By batching the two SB updates in a single transaction we
lower the risk of this situation happening.
CC: Numan Siddique
Fixes: 054f4c85c413 ("Add a new logical switch port type - 'virtual'")
Signed-off-by: Dumitru Ceara
---
controller/binding.c | 97 +---
1 file changed, 69 insertions(+), 28 deletions(-)
diff --git a/controller/binding.c b/controller/binding.c
index aad9d39..4c107c1 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -472,7 +472,12 @@ is_our_chassis(const struct sbrec_chassis *chassis_rec,
return our_chassis;
}
-static void
+/* Updates 'binding_rec' and if the port binding is local also updates the
+ * local datapaths and ports.
+ * Updates and returns the array of local virtual ports that will require
+ * additional processing.
+ */
+static const struct sbrec_port_binding **
consider_local_datapath(struct ovsdb_idl_txn *ovnsb_idl_txn,
struct ovsdb_idl_txn *ovs_idl_txn,
struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
@@ -485,7 +490,9 @@ consider_local_datapath(struct ovsdb_idl_txn *ovnsb_idl_txn,
struct hmap *local_datapaths,
struct shash *lport_to_iface,
struct sset *local_lports,
-struct sset *local_lport_ids)
+struct sset *local_lport_ids,
+const struct sbrec_port_binding **vpbs,
+size_t *n_vpbs, size_t *n_allocated_vpbs)
{
const struct ovsrec_interface *iface_rec
= shash_find_data(lport_to_iface, binding_rec->logical_port);
@@ -587,22 +594,11 @@ consider_local_datapath(struct ovsdb_idl_txn
*ovnsb_idl_txn,
}
} else if (binding_rec->chassis == chassis_rec) {
if (!strcmp(binding_rec->type, "virtual")) {
-/* pinctrl module takes care of binding the ports
- * of type 'virtual'.
- * Release such ports if their virtual parents are no
- * longer claimed by this chassis. */
-const struct sbrec_port_binding *parent
-= lport_lookup_by_name(sbrec_port_binding_by_name,
-binding_rec->virtual_parent);
-if (!parent || parent->chassis != chassis_rec) {
-VLOG_INFO("Releasing lport %s from this chassis.",
-binding_rec->logical_port);
-if (binding_rec->encap) {
-sbrec_port_binding_set_encap(binding_rec, NULL);
-}
-sbrec_port_binding_set_chassis(binding_rec, NULL);
-sbrec_port_binding_set_virtual_parent(binding_rec, NULL);
+if (*n_vpbs == *n_allocated_vpbs) {
+vpbs = x2nrealloc(vpbs, n_allocated_vpbs, sizeof *vpbs);
}
+vpbs[(*n_vpbs)] = binding_rec;
+(*n_vpbs)++;
} else {
VLOG_INFO("Releasing lport %s from this chassis.",
binding_rec->logical_port);
@@ -621,6 +617,30 @@ consider_local_datapath(struct ovsdb_idl_txn
*ovnsb_idl_txn,
vif_chassis);
}
}
+return vpbs;
+}
+
+static void
+consider_local_virtual_port(struct ovsdb_idl_index *sbrec_port_binding_by_name,
+const struct sbrec_chassis *chassis_rec,
+const struct sbrec_port_binding *binding_rec)
+{
+/* pinctrl module takes care of binding the ports of type 'virtual'.
+ * Release such ports if their virtual parents are no longer claimed by
+ * this chassis.
+ */
+const struct sbrec_port_binding *parent =
+lport_lookup_by_name(sbrec_port_binding_by_name,
+ binding_rec->virtual_parent);
+if (!parent || parent->chassis != chassis_rec) {
+VLOG_INFO("Releasing lport %s from this chassis.",
+ binding_rec->logical_port);
+if (binding_rec->encap) {
+
Re: [ovs-dev] [PATCH] dp-packet: Fix clearing/copying of memory layout flags.
On 22.11.2019 13:27, David Marchand wrote: > On Fri, Nov 22, 2019 at 1:15 PM Ilya Maximets wrote: >> >> On 22.11.2019 12:42, David Marchand wrote: >>> On Thu, Nov 21, 2019 at 2:59 PM Ilya Maximets wrote: 'ol_flags' of DPDK mbuf could contain bits responsible for external or indirect buffers which are not actually offload flags in a common sense. Clearing/copying of these flags could lead to memory leaks of external memory chunks and crashes due to access to wrong memory. OVS should not clear these flags while resetting offloads and also should not copy them to the newly allocated packets. This change is required to support DPDK 19.11, as some drivers may return mbufs with these flags set. However, it might be good to do the same for DPDK 18.11, because these flags are present and should be taken into account. >>> >>> Did you consider inverting the logic? >>> >>> Rather than exclude a set of flags, I would touch/copy only the flags >>> that OVS uses/understands. >>> >>> When OVS uses a DPDK flag, it has an associated API and meaning wrt >>> the rte_mbuf and the data. >>> - when the flags are reset in OVS, that's because something has been >>> done to the data (and the checksums and the rss hash must be >>> reevaluated), >>> - when a buffer is copied, OVS copies the data and the context that >>> matters to OVS, >>> >>> Anything else should be discarded when copying to a new dp-packet. >> >> Yes, that was the first version I wanted to implement, i.e. to collect >> all the flags that OVS really uses and clear/copy only these flags. >> >> But then I started to think about 'ring' ports and that we might send >> mbufs with incorrect flags set after the packet modification to the >> external application. This doesn't sound good. Because if OVS doesn't >> use them doesn't mean that other applications doesn't. So, I've end up >> with the current logic with clearing everything except the memory layout >> flags. >> >> Does it make sense? What do you think? > > Before sending a packet through a dpdk ring, OVS will touch the data > and update the relevant flags as far as it is concerned. > > The application can read/touch those mbuf flags as long as it complies > with the DPDK APIs, this concerns both the flags that OVS is aware of, > and the rest. > So when getting this mbuf back, the flags should be valid. > > After this, OVS can do what it wants on the packet, it will only touch > the flags it understands. > > What am I missing? I agree that OVS itself will work OK by only considering flags it really uses. I'm concerned about the second application that receives mbuf from the OVS via ring port. For example, I see that ixgbe driver almost unconditionally parses vlans and sets PKT_RX_VLAN along with mbuf->vlan_tci. If OVS will not clear this flag while sending to the ring port, other application that receives that mbuf will think that mbuf contains valid 'vlan_tci', which is not correct, because OVS might modify or strip that vlan from the packet before sending. > > >> BTW, it might be good to have a DPDK API for offload flags clear/get/copy. > > There is a rte_pktmbuf_copy that has been introduced recently, think a > helper copies metadata too... the best is to bother Olivier :-) > > >> >> BTW2, I don't really understand why memory layout flags are in ol_flags >> in the first place. I guess, there was just no room in mbuf structure? >> Is it possible that these flags will become regular or dynamic fields? > > I think it is the reason. > Again, Olivier ? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dp-packet: Fix clearing/copying of memory layout flags.
On Fri, Nov 22, 2019 at 01:27:57PM +0100, David Marchand wrote:
> On Fri, Nov 22, 2019 at 1:15 PM Ilya Maximets wrote:
> >
> > On 22.11.2019 12:42, David Marchand wrote:
> > > On Thu, Nov 21, 2019 at 2:59 PM Ilya Maximets wrote:
> > >>
> > >> 'ol_flags' of DPDK mbuf could contain bits responsible for external
> > >> or indirect buffers which are not actually offload flags in a common
> > >> sense. Clearing/copying of these flags could lead to memory leaks of
> > >> external memory chunks and crashes due to access to wrong memory.
> > >>
> > >> OVS should not clear these flags while resetting offloads and also
> > >> should not copy them to the newly allocated packets.
> > >>
> > >> This change is required to support DPDK 19.11, as some drivers may
> > >> return mbufs with these flags set. However, it might be good to do
> > >> the same for DPDK 18.11, because these flags are present and should
> > >> be taken into account.
> > >
> > > Did you consider inverting the logic?
> > >
> > > Rather than exclude a set of flags, I would touch/copy only the flags
> > > that OVS uses/understands.
> > >
> > > When OVS uses a DPDK flag, it has an associated API and meaning wrt
> > > the rte_mbuf and the data.
> > > - when the flags are reset in OVS, that's because something has been
> > > done to the data (and the checksums and the rss hash must be
> > > reevaluated),
> > > - when a buffer is copied, OVS copies the data and the context that
> > > matters to OVS,
> > >
> > > Anything else should be discarded when copying to a new dp-packet.
> >
> > Yes, that was the first version I wanted to implement, i.e. to collect
> > all the flags that OVS really uses and clear/copy only these flags.
> >
> > But then I started to think about 'ring' ports and that we might send
> > mbufs with incorrect flags set after the packet modification to the
> > external application. This doesn't sound good. Because if OVS doesn't
> > use them doesn't mean that other applications doesn't. So, I've end up
> > with the current logic with clearing everything except the memory layout
> > flags.
> >
> > Does it make sense? What do you think?
>
> Before sending a packet through a dpdk ring, OVS will touch the data
> and update the relevant flags as far as it is concerned.
>
> The application can read/touch those mbuf flags as long as it complies
> with the DPDK APIs, this concerns both the flags that OVS is aware of,
> and the rest.
> So when getting this mbuf back, the flags should be valid.
>
> After this, OVS can do what it wants on the packet, it will only touch
> the flags it understands.
>
> What am I missing?
>
>
> > BTW, it might be good to have a DPDK API for offload flags clear/get/copy.
>
> There is a rte_pktmbuf_copy that has been introduced recently, think a
> helper copies metadata too... the best is to bother Olivier :-)
There is no API for that. Currently, rte_pktmbuf_copy() does:
mc->ol_flags = m->ol_flags & ~(IND_ATTACHED_MBUF|EXT_ATTACHED_MBUF);
> > BTW2, I don't really understand why memory layout flags are in ol_flags
> > in the first place. I guess, there was just no room in mbuf structure?
> > Is it possible that these flags will become regular or dynamic fields?
>
> I think it is the reason.
> Again, Olivier ?
Historically, the ol_flags field was only used for offloads. But since
the introduction of such features, "flags" would have been a better
name. This is something we could do by adding a union { flags; ol_flags; }
and remove ol_flags at next API breakage.
Olivier
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 2/4] travis: Move x86-only addon packages
Hi Ilya,
Reply inline.
Best Regards,
Wei Yanqin
> -Original Message-
> From: dev On Behalf Of Ilya Maximets
> Sent: Friday, November 22, 2019 3:30 AM
> To: Lance Yang (Arm Technology China) ;
> d...@openvswitch.org; ovs-dev@openvswitch.org
> Cc: Jieqiang Wang (Arm Technology China) ;
> Ruifeng Wang (Arm Technology China) ; Gavin Hu
> (Arm Technology China) ; Jingzhao Ni (Arm Technology
> China) ; nd
> Subject: Re: [ovs-dev] [PATCH v1 2/4] travis: Move x86-only addon packages
>
> On 20.11.2019 9:14, Lance Yang wrote:
> > To enable multiple CPU architectures support, it is necessary to move
> > the x86-only addon packages from .travis.yml file. Otherwise, the
> > x86-only addon packages will break the builds on some other CPU
> architectures.
> >
> > Reviewed-by: Yangqin Wei
> > Reviewed-by: Malvika Gupta
> > Reviewed-by: Gavin Hu
> > Reviewed-by: Ruifeng Wang
> > Signed-off-by: Lance Yang
> > ---
> > .travis.yml | 2 --
> > .travis/linux-prepare.sh | 12
> > 2 files changed, 8 insertions(+), 6 deletions(-)
>
> Common comment for all the patches in a series:
> * It's better to add a period in the end of a subject line.
[Yanqin] OK.
>
> >
> > diff --git a/.travis.yml b/.travis.yml index 482efd2..2dc4d43 100644
> > --- a/.travis.yml
> > +++ b/.travis.yml
> > @@ -14,7 +14,6 @@ addons:
> >apt:
> > packages:
> >- bc
> > - - gcc-multilib
> >- libssl-dev
> >- llvm-dev
> >- libjemalloc1
> > @@ -26,7 +25,6 @@ addons:
> >- libelf-dev
> >- selinux-policy-dev
> >- libunbound-dev
> > - - libunbound-dev:i386
> >- libunwind-dev
> >
> > before_install: ./.travis/${TRAVIS_OS_NAME}-prepare.sh
> > diff --git a/.travis/linux-prepare.sh b/.travis/linux-prepare.sh index
> > 9e3ac0d..8096abe 100755
> > --- a/.travis/linux-prepare.sh
> > +++ b/.travis/linux-prepare.sh
> > @@ -15,10 +15,14 @@ cd ..
> > pip install --disable-pip-version-check --user six flake8 hacking
> > pip install --user --upgrade docutils
> >
> > -if [ "$M32" ]; then
> > -# 32-bit and 64-bit libunwind can not be installed at the same time.
> > -# This will remove the 64-bit libunwind and install 32-bit version.
> > -sudo apt-get install -y libunwind-dev:i386
> > +if [[ "$TRAVIS_ARCH" == "amd64" ]] || [[ -z "$TRAVIS_ARCH" ]]; then
>
> The same comment here as for previous ppc64le patch.
> Are you going to ever build 32bit binary on aarch64 on Travis?
> Is it really possible to build 32bit binary on aarch64 with '-m32' flag?
[Yanqin] Not yet. Gcc for aarch64 does not support -m32 flag. Cross compiler is
required to build 32 bits binary on aarch64 machine.
>
> > +if [ "$M32" ]; then
> > +# 32-bit and 64-bit libunwind can not be installed at the same
> > time.
> > +# This will remove the 64-bit libunwind and install 32-bit version.
> > +sudo apt-get install \
> > +-y libunwind-dev:i386 libunbound-dev:i386 gcc-multilib
>
> Please, add additional indentation level for above line.
[Yanqin] Thanks, will be updated in V2.
>
> > +fi
> > +
> > fi
> >
> > # IPv6 is supported by kernel but disabled in TravisCI images:
> >
> ___
> 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] dp-packet: Fix clearing/copying of memory layout flags.
On Fri, Nov 22, 2019 at 1:15 PM Ilya Maximets wrote: > > On 22.11.2019 12:42, David Marchand wrote: > > On Thu, Nov 21, 2019 at 2:59 PM Ilya Maximets wrote: > >> > >> 'ol_flags' of DPDK mbuf could contain bits responsible for external > >> or indirect buffers which are not actually offload flags in a common > >> sense. Clearing/copying of these flags could lead to memory leaks of > >> external memory chunks and crashes due to access to wrong memory. > >> > >> OVS should not clear these flags while resetting offloads and also > >> should not copy them to the newly allocated packets. > >> > >> This change is required to support DPDK 19.11, as some drivers may > >> return mbufs with these flags set. However, it might be good to do > >> the same for DPDK 18.11, because these flags are present and should > >> be taken into account. > > > > Did you consider inverting the logic? > > > > Rather than exclude a set of flags, I would touch/copy only the flags > > that OVS uses/understands. > > > > When OVS uses a DPDK flag, it has an associated API and meaning wrt > > the rte_mbuf and the data. > > - when the flags are reset in OVS, that's because something has been > > done to the data (and the checksums and the rss hash must be > > reevaluated), > > - when a buffer is copied, OVS copies the data and the context that > > matters to OVS, > > > > Anything else should be discarded when copying to a new dp-packet. > > Yes, that was the first version I wanted to implement, i.e. to collect > all the flags that OVS really uses and clear/copy only these flags. > > But then I started to think about 'ring' ports and that we might send > mbufs with incorrect flags set after the packet modification to the > external application. This doesn't sound good. Because if OVS doesn't > use them doesn't mean that other applications doesn't. So, I've end up > with the current logic with clearing everything except the memory layout > flags. > > Does it make sense? What do you think? Before sending a packet through a dpdk ring, OVS will touch the data and update the relevant flags as far as it is concerned. The application can read/touch those mbuf flags as long as it complies with the DPDK APIs, this concerns both the flags that OVS is aware of, and the rest. So when getting this mbuf back, the flags should be valid. After this, OVS can do what it wants on the packet, it will only touch the flags it understands. What am I missing? > BTW, it might be good to have a DPDK API for offload flags clear/get/copy. There is a rte_pktmbuf_copy that has been introduced recently, think a helper copies metadata too... the best is to bother Olivier :-) > > BTW2, I don't really understand why memory layout flags are in ol_flags > in the first place. I guess, there was just no room in mbuf structure? > Is it possible that these flags will become regular or dynamic fields? I think it is the reason. Again, Olivier ? -- David Marchand ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dp-packet: Fix clearing/copying of memory layout flags.
On 22.11.2019 12:42, David Marchand wrote: > On Thu, Nov 21, 2019 at 2:59 PM Ilya Maximets wrote: >> >> 'ol_flags' of DPDK mbuf could contain bits responsible for external >> or indirect buffers which are not actually offload flags in a common >> sense. Clearing/copying of these flags could lead to memory leaks of >> external memory chunks and crashes due to access to wrong memory. >> >> OVS should not clear these flags while resetting offloads and also >> should not copy them to the newly allocated packets. >> >> This change is required to support DPDK 19.11, as some drivers may >> return mbufs with these flags set. However, it might be good to do >> the same for DPDK 18.11, because these flags are present and should >> be taken into account. > > Did you consider inverting the logic? > > Rather than exclude a set of flags, I would touch/copy only the flags > that OVS uses/understands. > > When OVS uses a DPDK flag, it has an associated API and meaning wrt > the rte_mbuf and the data. > - when the flags are reset in OVS, that's because something has been > done to the data (and the checksums and the rss hash must be > reevaluated), > - when a buffer is copied, OVS copies the data and the context that > matters to OVS, > > Anything else should be discarded when copying to a new dp-packet. Yes, that was the first version I wanted to implement, i.e. to collect all the flags that OVS really uses and clear/copy only these flags. But then I started to think about 'ring' ports and that we might send mbufs with incorrect flags set after the packet modification to the external application. This doesn't sound good. Because if OVS doesn't use them doesn't mean that other applications doesn't. So, I've end up with the current logic with clearing everything except the memory layout flags. Does it make sense? What do you think? BTW, it might be good to have a DPDK API for offload flags clear/get/copy. BTW2, I don't really understand why memory layout flags are in ol_flags in the first place. I guess, there was just no room in mbuf structure? Is it possible that these flags will become regular or dynamic fields? Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dp-packet: Fix clearing/copying of memory layout flags.
On Thu, Nov 21, 2019 at 2:59 PM Ilya Maximets wrote: > > 'ol_flags' of DPDK mbuf could contain bits responsible for external > or indirect buffers which are not actually offload flags in a common > sense. Clearing/copying of these flags could lead to memory leaks of > external memory chunks and crashes due to access to wrong memory. > > OVS should not clear these flags while resetting offloads and also > should not copy them to the newly allocated packets. > > This change is required to support DPDK 19.11, as some drivers may > return mbufs with these flags set. However, it might be good to do > the same for DPDK 18.11, because these flags are present and should > be taken into account. Did you consider inverting the logic? Rather than exclude a set of flags, I would touch/copy only the flags that OVS uses/understands. When OVS uses a DPDK flag, it has an associated API and meaning wrt the rte_mbuf and the data. - when the flags are reset in OVS, that's because something has been done to the data (and the checksums and the rss hash must be reevaluated), - when a buffer is copied, OVS copies the data and the context that matters to OVS, Anything else should be discarded when copying to a new dp-packet. -- David Marchand ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 2/2] northd: Improve handling of pause and resume
On Fri, Nov 22, 2019 at 11:46 AM Frode Nordahl wrote: > > On Fri, Nov 22, 2019 at 11:12 AM Frode Nordahl > wrote: > > > > This is weird, the patches apply on an up-to-date ovn repo with git am > > on blobs from the list. > > > > I noticed the mailing list thread order is in reverse with patch > > number two preceding patch number one, could that be the issue? > > It may also be me editing the output from ``git format-patch`` prior to > submitting, but cannot reproduce the error reported by the bot locally > with blobs gleaned from list, so would love to hear your views on > what's going on here. Ah yes that must be it, the commit msg is part of the commit sha1 and apparently I have tampered with it, so this is my bad. Sorry about the noise, I'll submit an untampered v2. -- Frode Nordahl > -- > Frode Nordahl > > > -- > > Frode Nordahl > > > > On Fri, Nov 22, 2019 at 10:59 AM 0-day Robot wrote: > > > > > > Bleep bloop. Greetings Frode Nordahl, I am a robot and I have tried out > > > your patch. > > > Thanks for your contribution. > > > > > > I encountered some error that I wasn't expecting. See the details below. > > > > > > > > > git-am: > > > fatal: sha1 information is lacking or useless (tests/ovn-northd.at). > > > Repository lacks necessary blobs to fall back on 3-way merge. > > > Cannot fall back to three-way merge. > > > Patch failed at 0001 northd: Improve handling of pause and resume > > > 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
Re: [ovs-dev] [PATCH v1 1/4] dpif-netdev: Fix the pmd_perf_stats
On 22.11.2019 10:20, Lance Yang (Arm Technology China) wrote:
> Hi Ilya,
>
> Thanks for your comments. Please take time to see the inline comments.
>
> Best regards,
> Lance
>> -Original Message-
>> From: Ilya Maximets
>> Sent: Friday, November 22, 2019 3:23 AM
>> To: Lance Yang (Arm Technology China) ; ovs-
>> d...@openvswitch.org
>> Cc: Jieqiang Wang (Arm Technology China) ; Gavin Hu
>> (Arm
>> Technology China) ; Jingzhao Ni (Arm Technology China)
>> ; nd ; Ruifeng Wang (Arm Technology China)
>>
>> Subject: Re: [ovs-dev] [PATCH v1 1/4] dpif-netdev: Fix the pmd_perf_stats
>>
>> On 20.11.2019 9:13, Lance Yang wrote:
>>> When compiling Open vSwitch for aarch64, the compiler will warn about
>>> an uninitialized variable in lib/dpif-netdev-perf.c. It is necessary
>>> to initialize it to avoid build failure.
>>
>> Hi. Thanks for making OVS build on aarch64.
>>
>> Could you provide the error message?
>
> [Lance]
> For the error message you would like to see, I copied it below:
> --- Begin of the error message ---
> In file included from lib/dpif-netdev-perf.c:21:
> lib/dpif-netdev-perf.c: In function 'pmd_perf_estimate_tsc_frequency':
> lib/dpif-netdev-perf.h:198:16: error: 's.last_tsc' may be used uninitialized
> in this function [-Werror=maybe-uninitialized]
> return s->last_tsc;
>~^~
> libtool: compile: gcc -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib
> -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith
> -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter
> -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition
> -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing
> -Wswitch-bool -Wlogical-not-parentheses -Wsizeof-array-argument
> -Wbool-compare -Wshift-negative-value -Wduplicated-cond -Wshadow
> -Wmultistatement-macros -Wcast-align=strict -Werror -Werror -g -O2 -MT
> lib/heap.lo -MD -MP -MF lib/.deps/heap.Tpo -c lib/heap.c -o lib/heap.o
> cc1: all warnings being treated as errors
> --- end of the message---
>
> 191 static inline uint64_t
> 192 rdtsc_syscall(struct pmd_perf_stats *s)
> 193 {
> 194 struct timespec val;
> 195 uint64_t v;
> 196
> 197 if (clock_gettime(CLOCK_MONOTONIC_RAW, ) != 0) {
> 198return s->last_tsc;
> 199 }
> 200
> 201 v = val.tv_sec * UINT64_C(10) + val.tv_nsec;
> 202 return s->last_tsc = v;
> 203 }
> 204 #endif
>
> When the clock_gettime function failed, the code "return s->last_tsc" will be
> reached.
Oh, I see. So, this is not a false-positive even if it's not able
to produce any real issue.
Please, add this information to the commit message, i.e. describe in words
in which condition we could use this uninitialized value.
>> In fact this variable is never used (only assigned), so this should be a
>> false-positive. So, I'd
>> like to look at the error message.
>>
>
>> Also, it might be better to rename the patch to something more specific. e.g.
>> 'dpif-netdev-perf: Avoid false-positive on uninitialized perf stats.'
> I typed an "Enter key" in the patch. The original title should be:
>
> Subject: [ovs-dev] [PATCH v1 1/4] dpif-netdev: Fix the pmd_perf_stats
> uninitalized issue
s/uninitalized/uninitialized/
And you still need change the area from 'dpif-netdev' to 'dpif-netdev-perf'.
And a period in the end.
Maybe something like this:
'dpif-netdev-perf: Fix using of uninitialized last_tsc.' ?
>
> If this title looks fine, I will modify the title as above in the patch set
> v2.
>
>>>
>>> Reviewed-by: Yanqin Wei
>>> Signed-off-by: Lance Yang
>>> ---
>>> lib/dpif-netdev-perf.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/lib/dpif-netdev-perf.c b/lib/dpif-netdev-perf.c index
>>> baf90b0..f85bb0c 100644
>>> --- a/lib/dpif-netdev-perf.c
>>> +++ b/lib/dpif-netdev-perf.c
>>> @@ -63,6 +63,7 @@ pmd_perf_estimate_tsc_frequency(void)
>>> struct pmd_perf_stats s;
>>> uint64_t start, stop;
>>>
>>> +memset(, 0, sizeof(s));
>>
>> Please, don't parenthesize the argument of a 'sizeof'.
>>
>> Also, it might be better to initialize the variable close to the first
>> usage. It doesn't look
>> good here.
>>
> [Lance]
> I will remove the parenthesize for the sizeof operator and move the struct
> initialization closer to where it is used.
>
>>> /* DPDK is not available or returned unreliable value.
>>> * Trying to estimate. */
>>> affinity = ovs_numa_thread_getaffinity_dump();
>>>
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium. Thank you.
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 2/2] northd: Improve handling of pause and resume
On Fri, Nov 22, 2019 at 11:12 AM Frode Nordahl wrote: > > This is weird, the patches apply on an up-to-date ovn repo with git am > on blobs from the list. > > I noticed the mailing list thread order is in reverse with patch > number two preceding patch number one, could that be the issue? It may also be me editing the output from ``git format-patch`` prior to submitting, but cannot reproduce the error reported by the bot locally with blobs gleaned from list, so would love to hear your views on what's going on here. -- Frode Nordahl > -- > Frode Nordahl > > On Fri, Nov 22, 2019 at 10:59 AM 0-day Robot wrote: > > > > Bleep bloop. Greetings Frode Nordahl, I am a robot and I have tried out > > your patch. > > Thanks for your contribution. > > > > I encountered some error that I wasn't expecting. See the details below. > > > > > > git-am: > > fatal: sha1 information is lacking or useless (tests/ovn-northd.at). > > Repository lacks necessary blobs to fall back on 3-way merge. > > Cannot fall back to three-way merge. > > Patch failed at 0001 northd: Improve handling of pause and resume > > 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
Re: [ovs-dev] [PATCH v1 4/4] travis: Enable OVS Travis CI for arm
On 22.11.2019 10:35, Lance Yang (Arm Technology China) wrote: > Hi Ilya, > > Thanks for your reply. Please see the inline reply. > > Best regards, > Lance Yang > >> -Original Message- >> From: Ilya Maximets >> Sent: Friday, November 22, 2019 3:47 AM >> To: Lance Yang (Arm Technology China) ; ovs- >> d...@openvswitch.org >> Cc: Jieqiang Wang (Arm Technology China) ; Gavin Hu >> (Arm >> Technology China) ; Jingzhao Ni (Arm Technology China) >> ; dwil...@us.ibm.com; nd ; Ruifeng Wang >> (Arm >> Technology China) >> Subject: Re: [ovs-dev] [PATCH v1 4/4] travis: Enable OVS Travis CI for arm >> >> On 20.11.2019 9:15, Lance Yang wrote: >>> Enable part of travis jobs with gcc compiler for arm64 architecture >>> >>> 1. Add arm jobs into the matrix in .travis.yml and define dpdk cache >>> directory for arm64 architecture. >>> 2. Temporarily disable sparse checker because of static code checking >>> failure on arm64 >> >> Could you provide the sparse error messages? >> Maybe we could fix them instead of disabling. >> >>> >>> Successful travis build jobs report: >>> https://travis-ci.org/yzyuestc/ovs/builds/614299933 >> >> There are some issues in Travis in above build. It seems that Travis fails >> to upload the >> resulted cache directory due to missing md5deep: >> >> "" > [Lance] > The issue occurred because sparse checker does not recognize some arm neon > intrinsics: > > 1. Some components in Open vSwitch source code directly includes the header > file . > > 2. When compiling OvS with DPDK, OvS includes some DPDK headers, which > sometimes includes . You can see the details of the error in the > report at: https://travis-ci.org/yzyuestc/ovs/jobs/615414391 after line 2693 > > For the first issue where OvS directly imported the arm_neon.h, I can fix it > in my patch set v2. However, for the second issue that DPDK project has > imported the arm neon intrinsics, it seems that I am unable to fix this issue > until DPDK solved it. Hi. Thanks for the information. Could, you, please, check if the following patch fixes the issue: https://patchwork.ozlabs.org/patch/1199398/ ? > >> store build cache >> 0.02s5.41sInstalling md5deep >> Reading package lists... >> Building dependency tree... >> Reading state information... >> md5deep is already the newest version (4.4-2). >> 0 upgraded, 0 newly installed, 0 to remove and 8 not upgraded. >> /home/travis/.casher/bin/casher: line 230: md5deep: command not found nothing >> changed "" >> > [Lance] > Thanks for pointing out the issue. The upload cache on arm is not successful, > that means arm jobs will always clone and build DPDK. I just posted this > issue to Travis CI community. You can see: > https://travis-ci.community/t/no-cache-support-on-arm64/5416/21 Thanks! Let's wait for their reply. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] acinclude.m4: Define ARM vector instructions for cgcc.
We need to define __ARM_NEON, __ARM_NEON__ or __ARM_FEATURE_SIMD32 depending on the compiler and its version in order to make sparse happy while checking DPDK headers on ARM. This also will allow us to check same vectorized code that we're building. Reported-by: Lance Yang Signed-off-by: Ilya Maximets --- I didn't really test that this fixes all the sparse issues on ARM, at least it should fix some of them. Lance, could you, please, check? acinclude.m4 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acinclude.m4 b/acinclude.m4 index 542637ac8..40b842ae3 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -1256,7 +1256,7 @@ AC_DEFUN([OVS_CHECK_SPARSE_TARGET], dnl allow "sparse" correctly check the same code that will be built. dnl Required for checking DPDK headers. AC_MSG_CHECKING([vector options for cgcc]) - VECTOR=$($CC -dM -E - < /dev/null | grep -E "MMX|SSE|AVX" | \ + VECTOR=$($CC -dM -E - < /dev/null | grep -E "MMX|SSE|AVX|NEON|SIMD" | \ cut -c 9- | sed 's/ /=/' | sed 's/^/-D/' | tr '\n' ' ') AC_MSG_RESULT([$VECTOR]) CGCCFLAGS="$CGCCFLAGS $VECTOR" -- 2.17.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 2/2] northd: Improve handling of pause and resume
This is weird, the patches apply on an up-to-date ovn repo with git am on blobs from the list. I noticed the mailing list thread order is in reverse with patch number two preceding patch number one, could that be the issue? -- Frode Nordahl On Fri, Nov 22, 2019 at 10:59 AM 0-day Robot wrote: > > Bleep bloop. Greetings Frode Nordahl, I am a robot and I have tried out your > patch. > Thanks for your contribution. > > I encountered some error that I wasn't expecting. See the details below. > > > git-am: > fatal: sha1 information is lacking or useless (tests/ovn-northd.at). > Repository lacks necessary blobs to fall back on 3-way merge. > Cannot fall back to three-way merge. > Patch failed at 0001 northd: Improve handling of pause and resume > 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
Re: [ovs-dev] [PATCH ovn 2/2] northd: Improve handling of pause and resume
Bleep bloop. Greetings Frode Nordahl, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: fatal: sha1 information is lacking or useless (tests/ovn-northd.at). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 northd: Improve handling of pause and resume 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
Re: [ovs-dev] [PATCH v1 1/4] dpif-netdev: Fix the pmd_perf_stats
Hi Ilya,
Thanks for your comments. Please take time to see the inline comments.
Best regards,
Lance
> -Original Message-
> From: Ilya Maximets
> Sent: Friday, November 22, 2019 3:23 AM
> To: Lance Yang (Arm Technology China) ; ovs-
> d...@openvswitch.org
> Cc: Jieqiang Wang (Arm Technology China) ; Gavin Hu
> (Arm
> Technology China) ; Jingzhao Ni (Arm Technology China)
> ; nd ; Ruifeng Wang (Arm Technology China)
>
> Subject: Re: [ovs-dev] [PATCH v1 1/4] dpif-netdev: Fix the pmd_perf_stats
>
> On 20.11.2019 9:13, Lance Yang wrote:
> > When compiling Open vSwitch for aarch64, the compiler will warn about
> > an uninitialized variable in lib/dpif-netdev-perf.c. It is necessary
> > to initialize it to avoid build failure.
>
> Hi. Thanks for making OVS build on aarch64.
>
> Could you provide the error message?
[Lance]
For the error message you would like to see, I copied it below:
--- Begin of the error message ---
In file included from lib/dpif-netdev-perf.c:21:
lib/dpif-netdev-perf.c: In function 'pmd_perf_estimate_tsc_frequency':
lib/dpif-netdev-perf.h:198:16: error: 's.last_tsc' may be used uninitialized in
this function [-Werror=maybe-uninitialized]
return s->last_tsc;
~^~
libtool: compile: gcc -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib
-I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith
-Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast
-Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes
-Wmissing-field-initializers -fno-strict-aliasing -Wswitch-bool
-Wlogical-not-parentheses -Wsizeof-array-argument -Wbool-compare
-Wshift-negative-value -Wduplicated-cond -Wshadow -Wmultistatement-macros
-Wcast-align=strict -Werror -Werror -g -O2 -MT lib/heap.lo -MD -MP -MF
lib/.deps/heap.Tpo -c lib/heap.c -o lib/heap.o
cc1: all warnings being treated as errors
--- end of the message---
191 static inline uint64_t
192 rdtsc_syscall(struct pmd_perf_stats *s)
193 {
194 struct timespec val;
195 uint64_t v;
196
197 if (clock_gettime(CLOCK_MONOTONIC_RAW, ) != 0) {
198return s->last_tsc;
199 }
200
201 v = val.tv_sec * UINT64_C(10) + val.tv_nsec;
202 return s->last_tsc = v;
203 }
204 #endif
When the clock_gettime function failed, the code "return s->last_tsc" will be
reached.
> In fact this variable is never used (only assigned), so this should be a
> false-positive. So, I'd
> like to look at the error message.
>
> Also, it might be better to rename the patch to something more specific. e.g.
> 'dpif-netdev-perf: Avoid false-positive on uninitialized perf stats.'
I typed an "Enter key" in the patch. The original title should be:
Subject: [ovs-dev] [PATCH v1 1/4] dpif-netdev: Fix the pmd_perf_stats
uninitalized issue
If this title looks fine, I will modify the title as above in the patch set v2.
> >
> > Reviewed-by: Yanqin Wei
> > Signed-off-by: Lance Yang
> > ---
> > lib/dpif-netdev-perf.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/lib/dpif-netdev-perf.c b/lib/dpif-netdev-perf.c index
> > baf90b0..f85bb0c 100644
> > --- a/lib/dpif-netdev-perf.c
> > +++ b/lib/dpif-netdev-perf.c
> > @@ -63,6 +63,7 @@ pmd_perf_estimate_tsc_frequency(void)
> > struct pmd_perf_stats s;
> > uint64_t start, stop;
> >
> > +memset(, 0, sizeof(s));
>
> Please, don't parenthesize the argument of a 'sizeof'.
>
> Also, it might be better to initialize the variable close to the first usage.
> It doesn't look
> good here.
>
[Lance]
I will remove the parenthesize for the sizeof operator and move the struct
initialization closer to where it is used.
> > /* DPDK is not available or returned unreliable value.
> > * Trying to estimate. */
> > affinity = ovs_numa_thread_getaffinity_dump();
> >
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 3/4] travis: split cache and set target for
Hi Ilya, Thanks for your comments. > -Original Message- > From: Ilya Maximets > Sent: Friday, November 22, 2019 3:39 AM > To: Lance Yang (Arm Technology China) ; ovs- > d...@openvswitch.org > Cc: Jieqiang Wang (Arm Technology China) ; Gavin Hu > (Arm > Technology China) ; Jingzhao Ni (Arm Technology China) > ; dwil...@us.ibm.com; nd ; Ruifeng Wang > (Arm > Technology China) > Subject: Re: [ovs-dev] [PATCH v1 3/4] travis: split cache and set target for > > On 20.11.2019 9:15, Lance Yang wrote: > > To compile OvS with DPDK in some Travis jobs, it is necessary to set > > the build target and split the dpdk cache directory according to > > different CPU architectures. > > > > Reviewed-by: Yanqin Wei > > Reviewed-by: Malvika Gupta > > Signed-off-by: Lance Yang > > --- > > .travis/linux-build.sh | 32 +--- > > 1 file changed, 21 insertions(+), 11 deletions(-) > > > According to this thread: > https://travis-ci.community/t/no-cache-support-on-arm64/5416/20 > > The arch name should be included now in the cache key, so this should not be > an issue. > Could you, please, re-check? > [Lance] I rechecked Travis CI. I found the cache conflict issue was solved. But the upload cache function on arm is still not successful. I will remove cache split in v2. > Best regards, Ilya Maximets. IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 4/4] travis: Enable OVS Travis CI for arm
Hi Ilya, Thanks for your reply. Please see the inline reply. Best regards, Lance Yang > -Original Message- > From: Ilya Maximets > Sent: Friday, November 22, 2019 3:47 AM > To: Lance Yang (Arm Technology China) ; ovs- > d...@openvswitch.org > Cc: Jieqiang Wang (Arm Technology China) ; Gavin Hu > (Arm > Technology China) ; Jingzhao Ni (Arm Technology China) > ; dwil...@us.ibm.com; nd ; Ruifeng Wang > (Arm > Technology China) > Subject: Re: [ovs-dev] [PATCH v1 4/4] travis: Enable OVS Travis CI for arm > > On 20.11.2019 9:15, Lance Yang wrote: > > Enable part of travis jobs with gcc compiler for arm64 architecture > > > > 1. Add arm jobs into the matrix in .travis.yml and define dpdk cache > > directory for arm64 architecture. > > 2. Temporarily disable sparse checker because of static code checking > > failure on arm64 > > Could you provide the sparse error messages? > Maybe we could fix them instead of disabling. > > > > > Successful travis build jobs report: > > https://travis-ci.org/yzyuestc/ovs/builds/614299933 > > There are some issues in Travis in above build. It seems that Travis fails > to upload the > resulted cache directory due to missing md5deep: > > "" [Lance] The issue occurred because sparse checker does not recognize some arm neon intrinsics: 1. Some components in Open vSwitch source code directly includes the header file . 2. When compiling OvS with DPDK, OvS includes some DPDK headers, which sometimes includes . You can see the details of the error in the report at: https://travis-ci.org/yzyuestc/ovs/jobs/615414391 after line 2693 For the first issue where OvS directly imported the arm_neon.h, I can fix it in my patch set v2. However, for the second issue that DPDK project has imported the arm neon intrinsics, it seems that I am unable to fix this issue until DPDK solved it. > store build cache > 0.02s5.41sInstalling md5deep > Reading package lists... > Building dependency tree... > Reading state information... > md5deep is already the newest version (4.4-2). > 0 upgraded, 0 newly installed, 0 to remove and 8 not upgraded. > /home/travis/.casher/bin/casher: line 230: md5deep: command not found nothing > changed "" > [Lance] Thanks for pointing out the issue. The upload cache on arm is not successful, that means arm jobs will always clone and build DPDK. I just posted this issue to Travis CI community. You can see: https://travis-ci.community/t/no-cache-support-on-arm64/5416/21 > Best regards, Ilya Maximets. IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [bug-report] bfd decay unit case failure
Hi Ben, This issue is difficult to reproduce in local machine. We will try to do it. Could you suggest the log we need to collect or enable? Best Regards, Wei Yanqin > -Original Message- > From: dev On Behalf Of Ben Pfaff > Sent: Friday, November 22, 2019 1:34 AM > To: Lance Yang (Arm Technology China) > Cc: ovs-dev@openvswitch.org > Subject: Re: [ovs-dev] [bug-report] bfd decay unit case failure > > On Thu, Nov 21, 2019 at 09:25:49AM +, Lance Yang (Arm Technology > China) wrote: > > I encountered a unit test failure when I ran the Open vSwitch testsuite on > Travis CI aarch64 lxd container. > > The environment can be found at line 7 : build system information section in > the report on https://travis-ci.org/yzyuestc/ovs/jobs/614941322 . The unit > test > case name is "bfd decay" , you can find the unit test failure details in the > report after line 6520. The failure is not 100% reproducible on Travis CI. > > Could anyone give some hint on what is wrong for this unit test case? > > We do our best to make the Open vSwitch test cases resist differences in > timing from one environment to another, but there still may be some that are > sensitive to it. This one may be such a test case. These problems can be > difficult to debug without being able to trigger them in interactive > environments. Have you been able to see it when you build by hand? > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn 2/2] northd: Improve handling of pause and resume
Move paused state to ``struct northd_context`` and pass the
context to paused and status command handlers.
On pause release the OVSDB lock on SB DB.
Re-instante the lock on resume.
Status command will now provide accurate information for 'paused',
'active' and 'standby' states.
Merge separate status command test into the pause and resume tests.
Signed-off-by: Frode Nordahl
---
northd/ovn-northd.8.xml | 9 +++--
northd/ovn-northd.c | 87 +++--
tests/ovn-northd.at | 24 +++-
3 files changed, 79 insertions(+), 41 deletions(-)
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 9734e79e6..c6d5d96b9 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -74,13 +74,15 @@
pause
Pauses the ovn-northd operation from processing any Northbound and
-Southbound database changes.
+Southbound database changes. This will also instruct ovn-northd to
+drop any lock on SB DB.
resume
Resumes the ovn-northd operation to process Northbound and
-Southbound database contents and generate logical flows.
+Southbound database contents and generate logical flows. This will
+also instruct ovn-northd to aspire for the lock on SB DB.
is-paused
@@ -91,7 +93,8 @@
status
Prints this server's status. Status will be "active" if ovn-northd has
-acquired OVSDB lock on NB DB, "standby" otherwise.
+acquired OVSDB lock on SB DB, "standby" if it has not or "paused" if
+this instance is paused.
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index a943e1037..e19515d14 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -65,6 +65,7 @@ struct northd_context {
struct ovsdb_idl_index *sbrec_ha_chassis_grp_by_name;
struct ovsdb_idl_index *sbrec_mcast_group_by_name_dp;
struct ovsdb_idl_index *sbrec_ip_mcast_by_dp;
+bool paused;
};
/* An IPv4 or IPv6 address */
@@ -10836,6 +10837,8 @@ add_column_noalert(struct ovsdb_idl *idl,
ovsdb_idl_omit_alert(idl, column);
}
+#define OVN_NORTHD_SB_DB_LOCK_NAME "ovn_northd"
+
int
main(int argc, char *argv[])
{
@@ -10843,8 +10846,10 @@ main(int argc, char *argv[])
struct unixctl_server *unixctl;
int retval;
bool exiting;
-bool paused;
bool had_lock;
+struct northd_context ctx;
+
+memset(, 0, sizeof(ctx));
fatal_ignore_sigpipe();
ovs_cmdl_proctitle_init(argc, argv);
@@ -10866,11 +10871,11 @@ main(int argc, char *argv[])
exit(EXIT_FAILURE);
}
unixctl_command_register("exit", "", 0, 0, ovn_northd_exit, );
-unixctl_command_register("pause", "", 0, 0, ovn_northd_pause, );
-unixctl_command_register("resume", "", 0, 0, ovn_northd_resume, );
+unixctl_command_register("pause", "", 0, 0, ovn_northd_pause, );
+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);
+ );
+unixctl_command_register("status", "", 0, 0, ovn_northd_status, );
daemonize_complete();
@@ -11075,23 +11080,21 @@ main(int argc, char *argv[])
/* Ensure that only a single ovn-northd is active in the deployment by
* 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");
+ovsdb_idl_set_lock(ovnsb_idl_loop.idl, OVN_NORTHD_SB_DB_LOCK_NAME);
/* Main loop. */
exiting = false;
-paused = false;
+ctx.paused = false;
had_lock = false;
while (!exiting) {
-if (!paused) {
-struct northd_context ctx = {
-.ovnnb_idl = ovnnb_idl_loop.idl,
-.ovnnb_txn = ovsdb_idl_loop_run(_idl_loop),
-.ovnsb_idl = ovnsb_idl_loop.idl,
-.ovnsb_txn = ovsdb_idl_loop_run(_idl_loop),
-.sbrec_ha_chassis_grp_by_name = sbrec_ha_chassis_grp_by_name,
-.sbrec_mcast_group_by_name_dp = sbrec_mcast_group_by_name_dp,
-.sbrec_ip_mcast_by_dp = sbrec_ip_mcast_by_dp,
-};
+if (!ctx.paused) {
+ctx.ovnnb_idl = ovnnb_idl_loop.idl;
+ctx.ovnnb_txn = ovsdb_idl_loop_run(_idl_loop);
+ctx.ovnsb_idl = ovnsb_idl_loop.idl;
+ctx.ovnsb_txn = ovsdb_idl_loop_run(_idl_loop);
+ctx.sbrec_ha_chassis_grp_by_name = sbrec_ha_chassis_grp_by_name;
+ctx.sbrec_mcast_group_by_name_dp = sbrec_mcast_group_by_name_dp;
+ctx.sbrec_ip_mcast_by_dp = sbrec_ip_mcast_by_dp;
if (!had_lock && ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) {
VLOG_INFO("ovn-northd lock
[ovs-dev] [PATCH ovn 1/2] northd: Pause both ovn-northd instances in pause resume test
The current pause and resume tests rely on that ``ovn-northd`` will hold on to the lock while paused. Update the test to pause both ``ovn-northd`` instances so we may change this behaviour. Replace references to ``ovs-appctl`` with ``ovn-appctl``. Signed-off-by: Frode Nordahl --- tests/ovn-northd.at | 13 ++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index c73fd9003..1c94f2f65 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -912,7 +912,9 @@ AT_SETUP([ovn -- ovn-northd pause and resume]) AT_SKIP_IF([test $HAVE_PYTHON = no]) ovn_start -AT_CHECK([test xfalse = x`as northd ovs-appctl -t ovn-northd is-paused`]) +AT_CHECK([test xfalse = x`as northd ovn-appctl -t ovn-northd is-paused`]) +AT_CHECK([test xfalse = x`as northd-backup ovn-appctl -t ovn-northd \ +is-paused`]) ovn-nbctl ls-add sw0 @@ -927,7 +929,9 @@ OVS_WAIT_UNTIL([ # Now pause the ovn-northd as northd ovs-appctl -t ovn-northd pause -AT_CHECK([test xtrue = x`as northd ovs-appctl -t ovn-northd is-paused`]) +as northd-backup ovs-appctl -t ovn-northd pause +AT_CHECK([test xtrue = x`as northd ovn-appctl -t ovn-northd is-paused`]) +AT_CHECK([test xtrue = x`as northd-backup ovn-appctl -t ovn-northd is-paused`]) ovn-nbctl ls-add sw0 @@ -938,7 +942,10 @@ OVS_WAIT_UNTIL([ # Now resume ovn-northd as northd ovs-appctl -t ovn-northd resume -AT_CHECK([test xfalse = x`as northd ovs-appctl -t ovn-northd is-paused`]) +as northd-backup ovs-appctl -t ovn-northd resume +AT_CHECK([test xfalse = x`as northd ovn-appctl -t ovn-northd is-paused`]) +AT_CHECK([test xfalse = x`as northd-backup ovn-appctl -t ovn-northd \ +is-paused`]) OVS_WAIT_UNTIL([ ovn-sbctl lflow-list sw0 -- 2.24.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn 0/2] northd: Improve pause, resume and status
At present when the ``pause`` command is issued any lock the ``ovn-northd`` process holds on the SB DB will be kept. To be able to change this behaviour the tests surrounding pause and resume must be changed, thus I provide the proposed change as a series to demonstrate that the changed tests work both for the current code and the proposed code. Travis passed: https://travis-ci.org/fnordahl/ovn/builds/615446047 Frode Nordahl (2): northd: Pause both ovn-northd instances in pause resume test northd: Improve handling of pause and resume northd/ovn-northd.8.xml | 9 +++-- northd/ovn-northd.c | 87 +++-- tests/ovn-northd.at | 35 +++-- 3 files changed, 88 insertions(+), 43 deletions(-) -- 2.24.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
