Re: [ovs-dev] [PATCH ovn] Clear port binding flows when datapath CT zone changes.
On Fri, Nov 20, 2020 at 5:44 AM Mark Michelson wrote:
>
> In commit f9cab11d5fabe2ae321a3b4bad5972b61df958c0, a LOG_TO_PHY flow
> was changed so that it was no longer associated with a particular port
> binding. The logic there was that the particular flow contains data
> pertaining to the port binding's peer's datapath, so it didn't make
> sense to associate the flow with the port binding. This change was
> necessary in order for flows to be recalculated properly if the
> requested SNAT CT zone on a gateway router was changed. Since the
> datapath was changed but no port bindings were changed, that particular
> flow needed to be cleared so it could be recalculated with the new CT
> zones put in place.
>
> Unfortunately, that change broke some other behavior. Specifically, if a
> router was changed from a distributed router to a gateway router, then
> its port bindings and its port bindings' peers would be updated so that
> they were no longer type "patch" but instead type "l3gateway". They
> would attempt to remove all associated physical flows and then install
> the newly relevant ones. Since the LOG_TO_PHY flow was no longer
> associated with a port binding, that flow would remain. The result was
> that traffic could be sent to the gateway router on chassis where the
> gateway router was not pinned.
>
> This commit seeks to fix both behaviors. Now if CT zones are changed on
> a particular datapath, then all port bindings on that datapath, as well
> as all of those port bindings' peers will have their physical flows
> removed. When physical flows are recomputed, all of the appropriate
> flows will be added.
>
> Signed-off-by: Mark Michelson
> ---
> controller/ovn-controller.c | 31 ++--
> controller/physical.c | 58 +
> controller/physical.h | 4 +++
> 3 files changed, 79 insertions(+), 14 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 25de0c72f..eceb804ac 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -64,6 +64,7 @@
> #include "timer.h"
> #include "stopwatch.h"
> #include "lib/inc-proc-eng.h"
> +#include "hmapx.h"
>
> VLOG_DEFINE_THIS_MODULE(main);
>
> @@ -549,7 +550,7 @@ add_pending_ct_zone_entry(struct shash *pending_ct_zones,
> static void
> update_ct_zones(const struct sset *lports, const struct hmap
> *local_datapaths,
> struct simap *ct_zones, unsigned long *ct_zone_bitmap,
> -struct shash *pending_ct_zones)
> +struct shash *pending_ct_zones, struct hmapx *updated_dps)
> {
> struct simap_node *ct_zone, *ct_zone_next;
> int scan_start = 1;
> @@ -563,6 +564,7 @@ update_ct_zones(const struct sset *lports, const struct
> hmap *local_datapaths,
> }
>
> /* Local patched datapath (gateway routers) need zones assigned. */
> +struct shash all_lds = SHASH_INITIALIZER(_lds);
> const struct local_datapath *ld;
> HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
> /* XXX Add method to limit zone assignment to logical router
> @@ -571,6 +573,8 @@ update_ct_zones(const struct sset *lports, const struct
> hmap *local_datapaths,
> char *snat = alloc_nat_zone_key(>datapath->header_.uuid, "snat");
> sset_add(_users, dnat);
> sset_add(_users, snat);
> +shash_add(_lds, dnat, ld);
> +shash_add(_lds, snat, ld);
>
> int req_snat_zone = datapath_snat_ct_zone(ld->datapath);
> if (req_snat_zone >= 0) {
> @@ -636,6 +640,11 @@ update_ct_zones(const struct sset *lports, const struct
> hmap *local_datapaths,
>
> bitmap_set1(ct_zone_bitmap, snat_req_node->data);
> simap_put(ct_zones, snat_req_node->name, snat_req_node->data);
> +struct shash_node *ld_node = shash_find(_lds,
> snat_req_node->name);
> +if (ld_node) {
> +struct local_datapath *dp = ld_node->data;
> +hmapx_add(updated_dps, (void *) dp->datapath);
> +}
> }
>
> /* xxx This is wasteful to assign a zone to each port--even if no
> @@ -664,10 +673,17 @@ update_ct_zones(const struct sset *lports, const struct
> hmap *local_datapaths,
>
> bitmap_set1(ct_zone_bitmap, zone);
> simap_put(ct_zones, user, zone);
> +
> +struct shash_node *ld_node = shash_find(_lds, user);
> +if (ld_node) {
> +struct local_datapath *dp = ld_node->data;
> +hmapx_add(updated_dps, (void *) dp->datapath);
> +}
> }
>
> simap_destroy(_snat_zones);
> sset_destroy(_users);
> +shash_destroy(_lds);
> }
>
> static void
> @@ -1098,6 +1114,9 @@ struct ed_type_runtime_data {
> bool tracked;
> bool local_lports_changed;
> struct hmap tracked_dp_bindings;
> +
> +/* CT zone data. Contains datapaths that had updated CT zones */
> +struct hmapx ct_updated_datapaths;
Thanks for the fix.
The patch overall looks fine to
[ovs-dev] [PATCH] lib/tc: fix parse act pedit for tos rewrite
From: wenxu
Check overlap between current pedit key, which is always 4 bytes
(range [off, off + 3]), and a map entry in flower_pedit_map
sf = ROUND_DOWN(mf, 4) (range [sf|mf, (mf + sz - 1)|ef]).
So for the tos the rewite the off + 3(3) is greater than mf,
and should less than ef(4) but not mf+sz(2).
Signed-off-by: wenxu
---
lib/tc.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/lib/tc.c b/lib/tc.c
index 8761304..c2de78b 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -1003,6 +1003,7 @@ nl_parse_act_pedit(struct nlattr *options, struct
tc_flower *flower)
int flower_off = m->flower_offset;
int sz = m->size;
int mf = m->offset;
+int ef = ROUND_UP(mf, 4);
if (m->htype != type) {
continue;
@@ -1010,9 +1011,10 @@ nl_parse_act_pedit(struct nlattr *options, struct
tc_flower *flower)
/* check overlap between current pedit key, which is always
* 4 bytes (range [off, off + 3]), and a map entry in
- * flower_pedit_map (range [mf, mf + sz - 1]) */
+ * flower_pedit_map sf = ROUND_DOWN(mf, 4)
+ * (range [sf|mf, (mf + sz - 1)|ef]) */
if ((keys->off >= mf && keys->off < mf + sz)
-|| (keys->off + 3 >= mf && keys->off + 3 < mf + sz)) {
+|| (keys->off + 3 >= mf && keys->off + 3 < ef)) {
int diff = flower_off + (keys->off - mf);
ovs_be32 *dst = (void *) (rewrite_key + diff);
ovs_be32 *dst_m = (void *) (rewrite_mask + diff);
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] 在线挖掘采购信息
我已邀请您填写以下表单： 在线挖掘采购信息 要填写此表单，请访问： https://docs.google.com/forms/d/e/1FAIpQLSd2yJxTOYWWewvfJO9rjD6EDLUVymvM9_iQDGH_NXwQvuOcuQ/viewform?vc=0c=0w=1flr=0usp=mail_form_link 我已邀请您填写表单： Google表单：创建调查问卷并分析调查结果。 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net] net: openvswitch: fix TTL decrement action netlink message format
On Mon, 23 Nov 2020 20:36:39 +0100 Matteo Croce wrote:
> On Fri, Nov 20, 2020 at 10:12 PM Jakub Kicinski wrote:
> > On Thu, 19 Nov 2020 04:04:04 -0500 Eelco Chaudron wrote:
> > > Currently, the openvswitch module is not accepting the correctly formated
> > > netlink message for the TTL decrement action. For both setting and getting
> > > the dec_ttl action, the actions should be nested in the
> > > OVS_DEC_TTL_ATTR_ACTION attribute as mentioned in the openvswitch.h uapi.
> > >
> >
> > IOW this change will not break any known user space, correct?
> >
> > But existing OvS user space already expects it to work like you
> > make it work now?
> >
> > What's the harm in leaving it as is?
> >
> > > Fixes: 744676e77720 ("openvswitch: add TTL decrement action")
> > > Signed-off-by: Eelco Chaudron
> >
> > Can we get a review from OvS folks? Matteo looks good to you (as the
> > original author)?
>
> I think that the userspace still has to implement the dec_ttl action;
> by now dec_ttl is implemented with set_ttl().
> So there is no breakage yet.
>
> Eelco, with this fix we will encode the netlink attribute in the same
> way for the kernel and netdev datapath?
We don't allow breaking uAPI. Sounds like the user space never
implemented this and perhaps the nesting is just inconvenient
but not necessarily broken? If it is broken and unusable that
has to be clearly explained in the commit message. I'm dropping
v1 from patchwork.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] 答复: [PATCH V3 1/4] Enable VXLAN TSO for DPDK datapath
Flavio, thank you so much for clarification, I'll push "Enable VXLAN TSO for
DPDK datapath" first, replies for your comments inline, please check them in
the later part.
-邮件原件-
发件人: dev [mailto:ovs-dev-boun...@openvswitch.org] 代表 Flavio Leitner
发送时间: 2020年11月24日 3:10
收件人: yang_y_yi
抄送: ovs-dev@openvswitch.org
主题: Re: [ovs-dev] [PATCH V3 1/4] Enable VXLAN TSO for DPDK datapath
Hi Yi,
On Mon, Nov 02, 2020 at 11:16:49AM +0800, yang_y_yi wrote:
>
>
> Thanks a lot, Flavio, please check inline comments for more discussion.
>
>
>
> At 2020-10-31 01:55:57, "Flavio Leitner" wrote:
> >
> >Hi Yi,
> >
> >Thanks for the patch and sorry the delay to review it.
> >See my comments in line.
> >
> >Thanks,
> >fbl
> >
> >
> >On Fri, Aug 07, 2020 at 06:56:45PM +0800, yang_y...@163.com wrote:
> >> From: Yi Yang
> >>
> >> Many NICs can support VXLAN TSO which can help improve
> >> across-compute-node VM-to-VM performance in case that MTU is set to
> >> 1500.
> >>
> >> This patch allows dpdkvhostuserclient interface and veth/tap
> >> interface to leverage NICs' offload capability to maximize
> >> across-compute-node TCP performance, with it applied, OVS DPDK can
> >> reach linespeed for across-compute-node VM-to-VM TCP performance.
> >>
> >> Signed-off-by: Yi Yang
> >> ---
> >> lib/dp-packet.h | 76
> >> lib/netdev-dpdk.c | 188
> >> ++
> >> lib/netdev-linux.c| 20 ++
> >> lib/netdev-provider.h | 1 +
> >> lib/netdev.c | 69 --
> >> 5 files changed, 338 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/lib/dp-packet.h b/lib/dp-packet.h index
> >> 0430cca..79895f2 100644
> >> --- a/lib/dp-packet.h
> >> +++ b/lib/dp-packet.h
> >> @@ -81,6 +81,8 @@ enum dp_packet_offload_mask {
> >> DEF_OL_FLAG(DP_PACKET_OL_TX_UDP_CKSUM, PKT_TX_UDP_CKSUM, 0x400),
> >> /* Offload SCTP checksum. */
> >> DEF_OL_FLAG(DP_PACKET_OL_TX_SCTP_CKSUM, PKT_TX_SCTP_CKSUM,
> >> 0x800),
> >> +/* VXLAN TCP Segmentation Offload. */
> >> +DEF_OL_FLAG(DP_PACKET_OL_TX_TUNNEL_VXLAN, PKT_TX_TUNNEL_VXLAN,
> >> + 0x1000),
> >> /* Adding new field requires adding to
> >> DP_PACKET_OL_SUPPORTED_MASK. */ };
> >>
> >> @@ -1032,6 +1034,80 @@ dp_packet_hwol_set_tcp_seg(struct dp_packet *b)
> >> *dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_TCP_SEG; }
> >>
> >> +#ifdef DPDK_NETDEV
> >> +/* Mark packet 'b' for VXLAN TCP segmentation offloading. */
> >> +static inline void dp_packet_hwol_set_vxlan_tcp_seg(struct
> >> +dp_packet *b) {
> >> +b->mbuf.ol_flags |= DP_PACKET_OL_TX_TUNNEL_VXLAN;
> >> +b->mbuf.l2_len += sizeof(struct udp_header) +
> >> + sizeof(struct vxlanhdr);
> >
> >
> >What about L3 length?
>
> For tunnel offload, l2_len must be original l2_len plus vxlan and udp header
> size, l3_len is still be inner l3_len.
Ok, I see now that the patch requires DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM.
That feature is not very common, but might be fine to start with it,
and if needed add extra support for segmenting inner TCP only.
[Yi Yang] all the NICS which support TUNNEL offload can support
DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM, DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM will not be
used if no TUNNEL offload feature is available. For TUNNEL TCP segmenting, it
is also necessary, so it is a must-have feature if it can support TUNNEL
offload.
> >
> >> +b->mbuf.outer_l2_len = ETH_HEADER_LEN;
> >> +b->mbuf.outer_l3_len = IP_HEADER_LEN;
> >
> >What about IPv6?
>
> Good catch, we need to care outer ipv6 case. I'll split it to a single
> function dp_packet_hwol_set_outer_l2_len & dp_packet_hwol_set_l3_len to
> handle this.
Ok.
> >
> >> +}
> >> +
> >> +/* Check if it is a VXLAN packet */
> >> +static inline bool
> >> +dp_packet_hwol_is_vxlan_tcp_seg(struct dp_packet *b)
> >> +{
> >> +return (b->mbuf.ol_flags & DP_PACKET_OL_TX_TUNNEL_VXLAN);
> >
> >
> >Please use dp_packet_ol_flags_ptr()
>
> Ok, will use use dp_packet_ol_flags_ptr() in new version.
>
> >
> >> +}
> >> +
> >> +/* Set l2_len for the packet 'b' */
> >> +static inline void
> >> +dp_packet_hwol_set_l2_len(struct dp_packet *b, int l2_len)
> >> +{
> >> +b->mbuf.l2_len = l2_len;
> >> +}
> >
> >This function is only called by Linux in the ingress
> >path before the data processing, so it shouldn't set
> >any value other than the ones related to the iface
> >offloading at this point. Also that the data path can
> >change the packet and there is nothing to update
> >those lengths.
>
> Does "Linux" mean "system interfaces"?, we need to use l2_len, but I saw
> l2_len isn't set in some cases, so added this function.
Yes, system interfaces. See more details below.
> >In the egress path it calls netdev_dpdk_prep_hwol_packet()
> >to update those fields.
>
> If output port is system interfaces (veth or tap),
> netdev_dpdk_prep_hwol_packet() won't be called.
If the mbuf needs to be copied then you're
Re: [ovs-dev] (no subject)
-- The corona virus outbreak isn't just a major health crisis it's also a large economic disruption leading to people losing their jobs and making it harder to take care of their families. We've heard that a little financial support can go a long way. I'm Sheryll Goedert from Florida the Winner of a $396.9 million jackpot from the Power ball lottery held on January 29, 2020, My jackpot was a gift from God to me hence my entire family has AGREED to do this. We are donating €1,500,000.00 ( One Million Five Hundred Thousand Euro ) to help 10 individuals and small businesses. Contact me via my email:: goedert_she...@yahoo.com for further / full details and please accept this token as a gift from me and my family. Best Regards, Sheryll Goedert goedert_she...@yahoo.com ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH branch-2.11] stream-ssl: Make 'stream_ssl_set_key_and_cert' atomic
From 417ed338179d9856334e9b738abd71952581785a Mon Sep 17 00:00:00 2001
From: Thomas Neuman
Date: Mon, 23 Nov 2020 21:02:08 +
Subject: [PATCH branch-2.11] stream-ssl: Make 'stream_ssl_set_key_and_cert'
atomic
When attempting to set the SSL key and cert via this function,
first we check whether both the private key and certificate have
been changed, via a pair of calls to 'update_ssl_config'. However,
these calls modify the config which are being checked for changes.
In order for updates to be recognized atomically with respect to
the two files, we need to revert any changes made during the check.
Signed-off-by: Thomas Neuman
---
lib/stream-ssl.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/lib/stream-ssl.c b/lib/stream-ssl.c
index 343dced58..7bcc37864 100644
--- a/lib/stream-ssl.c
+++ b/lib/stream-ssl.c
@@ -1161,10 +1161,15 @@ void
stream_ssl_set_key_and_cert(const char *private_key_file,
const char *certificate_file)
{
-if (update_ssl_config(_key, private_key_file)
-&& update_ssl_config(, certificate_file)) {
-stream_ssl_set_certificate_file__(certificate_file);
-stream_ssl_set_private_key_file__(private_key_file);
+struct timespec orig_mtime = private_key.mtime;
+if (update_ssl_config(_key, private_key_file)) {
+if (update_ssl_config(, certificate_file)) {
+stream_ssl_set_certificate_file__(certificate_file);
+stream_ssl_set_private_key_file__(private_key_file);
+} else {
+// Revert the change performed by 'update_ssl_config'.
+private_key.mtime = orig_mtime;
+}
}
}
--
2.22.3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] TravisCI needs to be replaced.
On Mon, Nov 23, 2020 at 08:03:08PM +0100, Ilya Maximets wrote: > TL;DR OVS can not use Travis CI anymore. So, we need a new CI provider. > GitHub Actions looks like a sane replacement. Ilya, thank you for doing all of the research here and presenting all of it so thoroughly and clearly. It sounds like GitHub Actions is just fine for everything except ARM. I support moving to it, then, and we can figure out what to do about ARM later on. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net] net: openvswitch: fix TTL decrement action netlink message format
On Fri, Nov 20, 2020 at 10:12 PM Jakub Kicinski wrote:
>
> On Thu, 19 Nov 2020 04:04:04 -0500 Eelco Chaudron wrote:
> > Currently, the openvswitch module is not accepting the correctly formated
> > netlink message for the TTL decrement action. For both setting and getting
> > the dec_ttl action, the actions should be nested in the
> > OVS_DEC_TTL_ATTR_ACTION attribute as mentioned in the openvswitch.h uapi.
>
> IOW this change will not break any known user space, correct?
>
> But existing OvS user space already expects it to work like you
> make it work now?
>
> What's the harm in leaving it as is?
>
> > Fixes: 744676e77720 ("openvswitch: add TTL decrement action")
> > Signed-off-by: Eelco Chaudron
>
> Can we get a review from OvS folks? Matteo looks good to you (as the
> original author)?
>
Hi,
I think that the userspace still has to implement the dec_ttl action;
by now dec_ttl is implemented with set_ttl().
So there is no breakage yet.
Eelco, with this fix we will encode the netlink attribute in the same
way for the kernel and netdev datapath?
If so, go for it.
> > - err = __ovs_nla_copy_actions(net, attr, key, sfa, eth_type,
> > + err = __ovs_nla_copy_actions(net, actions, key, sfa, eth_type,
> >vlan_tci, mpls_label_count, log);
> > if (err)
> > return err;
>
> You're not canceling any nests on error, I assume this is normal.
>
> > + add_nested_action_end(*sfa, action_start);
> > add_nested_action_end(*sfa, start);
> > return 0;
> > }
>
--
per aspera ad upstream
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] TravisCI needs to be replaced.
Ilya Maximets writes: > Hi. > > TL;DR OVS can not use Travis CI anymore. So, we need a new CI provider. > GitHub Actions looks like a sane replacement. > > > As you, probably, noticed our Travis CI builds are taking lots of time to > start > and work. This is because of reduced capacity for travic-ci.org users. > > > What happened to travis-ci.org? > --- > Travis CI forces everyone to migrate to travic-ci.com. Their rationale is > that > it's easier for them to support one service and provide better support. Here > is > the only place where this info could be found: > > https://docs.travis-ci.com/user/migrate/open-source-repository-migration#frequently-asked-questions > > Right now travic-ci.org is completely unreliable since it takes too long for > build jobs to even by queued. They are waiting for days in 'job received' > state. > > travic-ci.org will be turned into read-only mode starting on December 31st. > > Before that time all users should migrate to travis-ci.com. > > > Why Open vSwitch project is not able to migrate to travis-ci.com > --- > In their blog post Travis CI says that they will convert all users who > currently > uses their services for free (like OVS) to their "Free" plan. But what does > it mean? This means that all these users will have 10.000 credits received > for > free and once these credits are gone, we'll need to buy new credits. > I tested this with my own github repo and it turned out that a single OVS > build > takes ~2.850 credits. i.e. we have 3 free builds. They also claim that > they're > still very supportive for OSS projects and that such projects could contact > their support and ask for special free OSS-only credits. OK, I did that. > It turned out that their definition of OSS project is slightly different. > In details, that is what I received in reply to my request for OSS-only > credits: > > Thanks for contacting Travis-CI Support! I’d love to help! We offer an > Open Source/Educational Subscription for free to non-commercial Open > Source > projects. To qualify for an OSS subscription, the project must meet the > following requirements: > > 1. You are a project lead or regular committer (latest commit in the >last month) > 2. Project must be at least 3 months old and is in active development >(with regular commits and activity) > 3. Project meets the OSD specification > 4. Project must not be sponsored by a commercial company or > organization >(monetary or with employees paid to work on the project) > 5. Project can not provide commercial services or distribute paid >versions of the software > > Sounds like you and your project? We’d be very happy to support you. If > your > project does not match these requirements or you have questions, feel free > to ask! > > To be honest, I do not think that any major FOSS project could meet > requirement > for not having employees of some company paid to work on the project. And OVS > clearly doesn't qualify for this. > > This means that we're not eligible for OSS-only credits. > > Blog link: https://blog.travis-ci.com/2020-11-02-travis-ci-new-billing > > The most frustrating part of all this is that they are not making any public > announces and you need to dig up all that information in forums and direct > conversations with their support. > > > What about paid service? > --- > Well, it's a tough question. In order to have same level of service as we had > we will need to pay for a plan with 5 concurrent jobs which is 250$/month. > And that is only for main openvswitch repo. But we also have ovsrobot and > maintainers and contributors who want's to test their work before pushing > to repo/sending patches upstream. So, this doesn't sound like a good solution > unless someone wants to spend few thousands per month on upstream CI. > > > What to do? > --- > Easiest solution right now seems to be migration to GitHub Actions. They are > providing free service for open repositories comparable with what Travis CI > provided. The main issue here is that GitHub Actions doesn't have support > for Arm. 2 ways to fix that: > - Use qemu-multiarch containers and test Arm builds in emulation. >Pros: Generic github runners. Cons: Might be slow. > - GitHub Actions has support for external Arm based runners. So we'll need >to find someone to host runner on their hardware. >Pros: Native builds. Cons: Someone needs to dedicate HW and time. > - Another CI provider. If you know free CI provider that could cover Arm, >please, suggest. > For now, until the end of December we could keep Arm jobs on travic-ci.org > and hope that it will eventually test them. > > > Immediate actions: > --- > Right now I'm blocked on testing patches since I practically have no CI to > back me up. So, I'm holding on everything right now to prepare initial > version > of GitHub
Re: [ovs-dev] [PATCH V3 1/4] Enable VXLAN TSO for DPDK datapath
Hi Yi,
On Mon, Nov 02, 2020 at 11:16:49AM +0800, yang_y_yi wrote:
>
>
> Thanks a lot, Flavio, please check inline comments for more discussion.
>
>
>
> At 2020-10-31 01:55:57, "Flavio Leitner" wrote:
> >
> >Hi Yi,
> >
> >Thanks for the patch and sorry the delay to review it.
> >See my comments in line.
> >
> >Thanks,
> >fbl
> >
> >
> >On Fri, Aug 07, 2020 at 06:56:45PM +0800, yang_y...@163.com wrote:
> >> From: Yi Yang
> >>
> >> Many NICs can support VXLAN TSO which can help
> >> improve across-compute-node VM-to-VM performance
> >> in case that MTU is set to 1500.
> >>
> >> This patch allows dpdkvhostuserclient interface
> >> and veth/tap interface to leverage NICs' offload
> >> capability to maximize across-compute-node TCP
> >> performance, with it applied, OVS DPDK can reach
> >> linespeed for across-compute-node VM-to-VM TCP
> >> performance.
> >>
> >> Signed-off-by: Yi Yang
> >> ---
> >> lib/dp-packet.h | 76
> >> lib/netdev-dpdk.c | 188
> >> ++
> >> lib/netdev-linux.c| 20 ++
> >> lib/netdev-provider.h | 1 +
> >> lib/netdev.c | 69 --
> >> 5 files changed, 338 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> >> index 0430cca..79895f2 100644
> >> --- a/lib/dp-packet.h
> >> +++ b/lib/dp-packet.h
> >> @@ -81,6 +81,8 @@ enum dp_packet_offload_mask {
> >> DEF_OL_FLAG(DP_PACKET_OL_TX_UDP_CKSUM, PKT_TX_UDP_CKSUM, 0x400),
> >> /* Offload SCTP checksum. */
> >> DEF_OL_FLAG(DP_PACKET_OL_TX_SCTP_CKSUM, PKT_TX_SCTP_CKSUM, 0x800),
> >> +/* VXLAN TCP Segmentation Offload. */
> >> +DEF_OL_FLAG(DP_PACKET_OL_TX_TUNNEL_VXLAN, PKT_TX_TUNNEL_VXLAN,
> >> 0x1000),
> >> /* Adding new field requires adding to DP_PACKET_OL_SUPPORTED_MASK. */
> >> };
> >>
> >> @@ -1032,6 +1034,80 @@ dp_packet_hwol_set_tcp_seg(struct dp_packet *b)
> >> *dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_TCP_SEG;
> >> }
> >>
> >> +#ifdef DPDK_NETDEV
> >> +/* Mark packet 'b' for VXLAN TCP segmentation offloading. */
> >> +static inline void
> >> +dp_packet_hwol_set_vxlan_tcp_seg(struct dp_packet *b)
> >> +{
> >> +b->mbuf.ol_flags |= DP_PACKET_OL_TX_TUNNEL_VXLAN;
> >> +b->mbuf.l2_len += sizeof(struct udp_header) +
> >> + sizeof(struct vxlanhdr);
> >
> >
> >What about L3 length?
>
> For tunnel offload, l2_len must be original l2_len plus vxlan and udp header
> size, l3_len is still be inner l3_len.
Ok, I see now that the patch requires DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM.
That feature is not very common, but might be fine to start with it,
and if needed add extra support for segmenting inner TCP only.
> >
> >> +b->mbuf.outer_l2_len = ETH_HEADER_LEN;
> >> +b->mbuf.outer_l3_len = IP_HEADER_LEN;
> >
> >What about IPv6?
>
> Good catch, we need to care outer ipv6 case. I'll split it to a single
> function dp_packet_hwol_set_outer_l2_len & dp_packet_hwol_set_l3_len to
> handle this.
Ok.
> >
> >> +}
> >> +
> >> +/* Check if it is a VXLAN packet */
> >> +static inline bool
> >> +dp_packet_hwol_is_vxlan_tcp_seg(struct dp_packet *b)
> >> +{
> >> +return (b->mbuf.ol_flags & DP_PACKET_OL_TX_TUNNEL_VXLAN);
> >
> >
> >Please use dp_packet_ol_flags_ptr()
>
> Ok, will use use dp_packet_ol_flags_ptr() in new version.
>
> >
> >> +}
> >> +
> >> +/* Set l2_len for the packet 'b' */
> >> +static inline void
> >> +dp_packet_hwol_set_l2_len(struct dp_packet *b, int l2_len)
> >> +{
> >> +b->mbuf.l2_len = l2_len;
> >> +}
> >
> >This function is only called by Linux in the ingress
> >path before the data processing, so it shouldn't set
> >any value other than the ones related to the iface
> >offloading at this point. Also that the data path can
> >change the packet and there is nothing to update
> >those lengths.
>
> Does "Linux" mean "system interfaces"?, we need to use l2_len, but I saw
> l2_len isn't set in some cases, so added this function.
Yes, system interfaces. See more details below.
> >In the egress path it calls netdev_dpdk_prep_hwol_packet()
> >to update those fields.
>
> If output port is system interfaces (veth or tap),
> netdev_dpdk_prep_hwol_packet() won't be called.
If the mbuf needs to be copied then you're right.
That's a bug which needs a separate patch. Let me know
if you want to do it, or if I could do it. I have a
refactoring in progress, but either way is fine by me.
> >> +
> >> +/* Set l3_len for the packet 'b' */
> >> +static inline void
> >> +dp_packet_hwol_set_l3_len(struct dp_packet *b, int l3_len)
> >> +{
> >> +b->mbuf.l3_len = l3_len;
> >> +}
> >
> >The same comment above is valid here.
>
> In some cases, we do need to set l3_len if it isn't set correctly.
>
> >
> >
> >> +
> >> +/* Set l4_len for the packet 'b' */
> >> +static inline void
> >> +dp_packet_hwol_set_l4_len(struct dp_packet *b, int l4_len)
> >> +{
> >> +b->mbuf.l4_len = l4_len;
> >> +}
> >
> >
>
[ovs-dev] TravisCI needs to be replaced.
Hi. TL;DR OVS can not use Travis CI anymore. So, we need a new CI provider. GitHub Actions looks like a sane replacement. As you, probably, noticed our Travis CI builds are taking lots of time to start and work. This is because of reduced capacity for travic-ci.org users. What happened to travis-ci.org? --- Travis CI forces everyone to migrate to travic-ci.com. Their rationale is that it's easier for them to support one service and provide better support. Here is the only place where this info could be found: https://docs.travis-ci.com/user/migrate/open-source-repository-migration#frequently-asked-questions Right now travic-ci.org is completely unreliable since it takes too long for build jobs to even by queued. They are waiting for days in 'job received' state. travic-ci.org will be turned into read-only mode starting on December 31st. Before that time all users should migrate to travis-ci.com. Why Open vSwitch project is not able to migrate to travis-ci.com? --- In their blog post Travis CI says that they will convert all users who currently uses their services for free (like OVS) to their "Free" plan. But what does it mean? This means that all these users will have 10.000 credits received for free and once these credits are gone, we'll need to buy new credits. I tested this with my own github repo and it turned out that a single OVS build takes ~2.850 credits. i.e. we have 3 free builds. They also claim that they're still very supportive for OSS projects and that such projects could contact their support and ask for special free OSS-only credits. OK, I did that. It turned out that their definition of OSS project is slightly different. In details, that is what I received in reply to my request for OSS-only credits: Thanks for contacting Travis-CI Support! I’d love to help! We offer an Open Source/Educational Subscription for free to non-commercial Open Source projects. To qualify for an OSS subscription, the project must meet the following requirements: 1. You are a project lead or regular committer (latest commit in the last month) 2. Project must be at least 3 months old and is in active development (with regular commits and activity) 3. Project meets the OSD specification 4. Project must not be sponsored by a commercial company or organization (monetary or with employees paid to work on the project) 5. Project can not provide commercial services or distribute paid versions of the software Sounds like you and your project? We’d be very happy to support you. If your project does not match these requirements or you have questions, feel free to ask! To be honest, I do not think that any major FOSS project could meet requirement for not having employees of some company paid to work on the project. And OVS clearly doesn't qualify for this. This means that we're not eligible for OSS-only credits. Blog link: https://blog.travis-ci.com/2020-11-02-travis-ci-new-billing The most frustrating part of all this is that they are not making any public announces and you need to dig up all that information in forums and direct conversations with their support. What about paid service? --- Well, it's a tough question. In order to have same level of service as we had we will need to pay for a plan with 5 concurrent jobs which is 250$/month. And that is only for main openvswitch repo. But we also have ovsrobot and maintainers and contributors who want's to test their work before pushing to repo/sending patches upstream. So, this doesn't sound like a good solution unless someone wants to spend few thousands per month on upstream CI. What to do? --- Easiest solution right now seems to be migration to GitHub Actions. They are providing free service for open repositories comparable with what Travis CI provided. The main issue here is that GitHub Actions doesn't have support for Arm. 2 ways to fix that: - Use qemu-multiarch containers and test Arm builds in emulation. Pros: Generic github runners. Cons: Might be slow. - GitHub Actions has support for external Arm based runners. So we'll need to find someone to host runner on their hardware. Pros: Native builds. Cons: Someone needs to dedicate HW and time. - Another CI provider. If you know free CI provider that could cover Arm, please, suggest. For now, until the end of December we could keep Arm jobs on travic-ci.org and hope that it will eventually test them. Immediate actions: --- Right now I'm blocked on testing patches since I practically have no CI to back me up. So, I'm holding on everything right now to prepare initial version of GitHub Actions migration. This doesn't mean that this will definitely be accepted, but at least this will allow me to work normally. I'll send patch to ovs-dev as soon as it is ready. Best regards, Ilya Maximets.
Re: [ovs-dev] [PATCH v3] python: Update build system to ensure dirs.py is created
On 19/11/2020 08:44, Mark Gray wrote: > Update build system to ensure dirs.py is created when it is a > dependency for a build target. Also, update setup.py to > check for that dependency. > > Signed-off-by: Mark Gray FWIW: Here is a link to a passing travis build: https://travis-ci.com/github/markdgray/ovs/builds/202451550 and appveyor https://ci.appveyor.com/project/markdgray/ovs-9qglw/builds/36384631 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] binding: Cleanup gateway port local binding in runtime data.
On Fri, Nov 20, 2020 at 9:08 PM Dumitru Ceara wrote:
>
> When a port binding of type "l3gateway" is claimed its remote peer
> port_binding is also stored in local_datapath.peer_ports[].remote.
>
> If the remote peer port_binding is deleted first (i.e., before the local
> "l3gateway" one) then we need to remove the complete
> local_datapath.peer_ports[] entry in order to avoid ending up using
> dangling pointers to already freed port bindings.
>
> Also, properly reset local_datapath->has_local_l3gateway in
> remove_pb_from_local_datapath().
>
> Ilya reported this issue found by AddressSanitizer during his testing:
>
> ==1816017==ERROR: AddressSanitizer: heap-use-after-free on address
> 0x614cb170 at pc 0x005ab574 bp 0x7fff68925a30 sp 0x7fff68925a28
> READ of size 8 at 0x614cb170 thread T0
> #0 0x5ab573 in put_replace_chassis_mac_flows
> git/ovn/controller/physical.c:550:9
> #1 0x5a65eb in consider_port_binding git/ovn/controller/physical.c:1168:13
> #2 0x5a8764 in physical_run git/ovn/controller/physical.c:1607:9
> #3 0x5a0064 in flow_output_physical_flow_changes_handler
> git/ovn/controller/ovn-controller.c:2127:9
> #4 0x5db423 in engine_compute git/ovn/lib/inc-proc-eng.c:306:18
> #5 0x5dae1f in engine_run_node git/ovn/lib/inc-proc-eng.c:352:14
> #6 0x5dac74 in engine_run git/ovn/lib/inc-proc-eng.c:377:9
> #7 0x59ad64 in main git/ovn/controller/ovn-controller.c
> #8 0x7f39fa6491a2 in __libc_start_main (/lib64/libc.so.6+0x271a2)
> #9 0x480b2d in _start (git/ovn/controller/ovn-controller+0x480b2d)
>
> 0x614cb170 is located 304 bytes inside of 408-byte region
> [0x614cb040,0x614cb1d8)
> freed by thread T0 here:
> #0 0x520d07 in free (git/ovn/controller/ovn-controller+0x520d07)
> #1 0x712de7 in ovsdb_idl_db_track_clear git/ovs/lib/ovsdb-idl.c:1984:21
> #2 0x59b5cd in main git/ovn/controller/ovn-controller.c:2762:9
> #3 0x7f39fa6491a2 in __libc_start_main (/lib64/libc.so.6+0x271a2)
>
> Reported-by: Ilya Maximets
> Fixes: 354bdba51abf ("ovn-controller: I-P for SB port binding and OVS
> interface in runtime_data.")
> Signed-off-by: Dumitru Ceara
Hi Dumitru,
Please see below for few comments.
> ---
> controller/binding.c | 47 ++-
> 1 file changed, 42 insertions(+), 5 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index eb92679..cb60c5d 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -1523,21 +1523,41 @@ binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
> }
>
> static const struct sbrec_port_binding *
> -get_peer_lport(const struct sbrec_port_binding *pb,
> - struct binding_ctx_in *b_ctx_in)
> +get_peer_lport__(const struct sbrec_port_binding *pb,
> + struct binding_ctx_in *b_ctx_in)
> {
> const char *peer_name = smap_get(>options, "peer");
> -if (strcmp(pb->type, "patch") || !peer_name) {
> +
> +if (!peer_name) {
> return NULL;
> }
>
> const struct sbrec_port_binding *peer;
> peer = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name,
> peer_name);
> -
> return (peer && peer->datapath) ? peer : NULL;
> }
>
> +static const struct sbrec_port_binding *
> +get_l3gw_peer_lport(const struct sbrec_port_binding *pb,
> +struct binding_ctx_in *b_ctx_in)
> +{
> +if (strcmp(pb->type, "l3gateway")) {
> +return NULL;
> +}
> +return get_peer_lport__(pb, b_ctx_in);
> +}
> +
> +static const struct sbrec_port_binding *
> +get_peer_lport(const struct sbrec_port_binding *pb,
> + struct binding_ctx_in *b_ctx_in)
> +{
> +if (strcmp(pb->type, "patch")) {
> +return NULL;
> +}
> +return get_peer_lport__(pb, b_ctx_in);
> +}
> +
> /* This function adds the local datapath of the 'peer' of
> * lport 'pb' to the local datapaths if it is not yet added.
> */
> @@ -1654,7 +1674,9 @@ remove_pb_from_local_datapath(const struct
> sbrec_port_binding *pb,
> pb->logical_port)) {
> ld->localnet_port = NULL;
> }
> -} else if (!strcmp(pb->type, "l3gateway")) {
> +}
This makes sense. Thanks for spotting this.
> +
> +if (!strcmp(pb->type, "l3gateway")) {
> const char *chassis_id = smap_get(>options,
>"l3gateway-chassis");
> if (chassis_id && !strcmp(chassis_id, chassis_rec->name)) {
> @@ -1956,12 +1978,27 @@ handle_deleted_lport(const struct sbrec_port_binding
> *pb,
> struct binding_ctx_in *b_ctx_in,
> struct binding_ctx_out *b_ctx_out)
> {
> +/* If the binding is local, remove it. */
> struct local_datapath *ld =
> get_local_datapath(b_ctx_out->local_datapaths,
> pb->datapath->tunnel_key);
> if (ld) {
>
Re: [ovs-dev] [PATCH] tests: Add overflow test for the sha1 library.
Ilya Maximets writes:
> This is a unit test for the overflow detection issue fixed by commit
> a1d2c5f5d9ed ("sha1: Fix algorithm for data bigger than 512 megabytes.")
>
> Signed-off-by: Ilya Maximets
> ---
Acked-by: Paolo Valerio
Tested-by: Paolo Valerio
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] netlink: removed incorrect optimization
I would expect such checks to be commented as indeed it was not clear why
it was there. I looked in similar places where FLOW_TNL_F_UDPIF is used
and there is no such check. The commit which created this condition is
quite large so I am not able to conclude from that.
I assume this is an optimization because if we don't set
OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS there is less code to be executed later
when the parsing is done. I believe the assumption was made to exclude this
without considering specifics of Geneve related FLOW_TNL_F_UDPIF flag.
On Fri, Nov 20, 2020 at 8:52 AM Ben Pfaff wrote:
> On Fri, Nov 20, 2020 at 05:12:46AM -0800, Toms Atteka wrote:
> > This optimization caused FLOW_TNL_F_UDPIF flag not to be used in
> > hash calculation for geneve tunnel when revalidating flows which
> > resulted in different cache hash values and incorrect behaviour.
> >
> > Reported-at: https://github.com/vmware-tanzu/antrea/issues/897
> > Signed-off-by: Toms Atteka
>
> Why was the check there? It seems strange to have a very specific "if"
> test and then just remove it without some idea of why it was there to
> begin with.
>
> ...
>
> > -} else if (flow->metadata.present.len || is_mask) {
> > +} else {
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] odp-util: Fix netlink message overflow with userdata.
Too big userdata could overflow netlink message leading to out-of-bound
memory accesses or assertion while formatting nested actions.
Fix that by checking the saize and returning correct error code.
Credit to OSS-Fuzz.
Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=27640
Fixes: e995e3df57ea ("Allow OVS_USERSPACE_ATTR_USERDATA to be variable length.")
Signed-off-by: Ilya Maximets
---
lib/odp-util.c | 30 +
lib/odp-util.h | 10 +-
ofproto/ofproto-dpif-xlate.c | 12 ++--
tests/odp.at | 37
4 files changed, 70 insertions(+), 19 deletions(-)
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 252a91bfa..879dea97e 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -1455,14 +1455,20 @@ parse_odp_userspace_action(const char *s, struct ofpbuf
*actions)
int n1 = -1;
if (ovs_scan([n], ",tunnel_out_port=%"SCNi32")%n",
_out_port, )) {
-odp_put_userspace_action(pid, user_data, user_data_size,
- tunnel_out_port, include_actions,
actions);
-res = n + n1;
+res = odp_put_userspace_action(pid, user_data, user_data_size,
+ tunnel_out_port, include_actions,
+ actions);
+if (res >= 0) {
+res = n + n1;
+}
goto out;
} else if (s[n] == ')') {
-odp_put_userspace_action(pid, user_data, user_data_size,
- ODPP_NONE, include_actions, actions);
-res = n + 1;
+res = odp_put_userspace_action(pid, user_data, user_data_size,
+ ODPP_NONE, include_actions,
+ actions);
+if (res >= 0) {
+res = n + 1;
+}
goto out;
}
}
@@ -7559,8 +7565,10 @@ odp_key_fitness_to_string(enum odp_key_fitness fitness)
* Netlink PID 'pid'. If 'userdata' is nonnull, adds a userdata attribute
* whose contents are the 'userdata_size' bytes at 'userdata' and returns the
* offset within 'odp_actions' of the start of the cookie. (If 'userdata' is
- * null, then the return value is not meaningful.) */
-size_t
+ * null, then the return value is not meaningful.)
+ *
+ * Returns negative error code on failure. */
+int
odp_put_userspace_action(uint32_t pid,
const void *userdata, size_t userdata_size,
odp_port_t tunnel_out_port,
@@ -7573,6 +7581,9 @@ odp_put_userspace_action(uint32_t pid,
offset = nl_msg_start_nested(odp_actions, OVS_ACTION_ATTR_USERSPACE);
nl_msg_put_u32(odp_actions, OVS_USERSPACE_ATTR_PID, pid);
if (userdata) {
+if (nl_attr_oversized(userdata_size)) {
+return -E2BIG;
+}
userdata_ofs = odp_actions->size + NLA_HDRLEN;
/* The OVS kernel module before OVS 1.11 and the upstream Linux kernel
@@ -7598,6 +7609,9 @@ odp_put_userspace_action(uint32_t pid,
if (include_actions) {
nl_msg_put_flag(odp_actions, OVS_USERSPACE_ATTR_ACTIONS);
}
+if (nl_attr_oversized(odp_actions->size - offset - NLA_HDRLEN)) {
+return -E2BIG;
+}
nl_msg_end_nested(odp_actions, offset);
return userdata_ofs;
diff --git a/lib/odp-util.h b/lib/odp-util.h
index 623a66aa2..46593c411 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -356,11 +356,11 @@ struct user_action_cookie {
};
BUILD_ASSERT_DECL(sizeof(struct user_action_cookie) == 48);
-size_t odp_put_userspace_action(uint32_t pid,
-const void *userdata, size_t userdata_size,
-odp_port_t tunnel_out_port,
-bool include_actions,
-struct ofpbuf *odp_actions);
+int odp_put_userspace_action(uint32_t pid,
+ const void *userdata, size_t userdata_size,
+ odp_port_t tunnel_out_port,
+ bool include_actions,
+ struct ofpbuf *odp_actions);
void odp_put_tunnel_action(const struct flow_tnl *tunnel,
struct ofpbuf *odp_actions,
const char *tnl_type);
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 11aa20754..9171290e0 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3222,12 +3222,12 @@ compose_sample_action(struct xlate_ctx *ctx,
odp_port_t odp_port = ofp_port_to_odp_port(
ctx->xbridge, ctx->xin->flow.in_port.ofp_port);
uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port);
-size_t cookie_offset = odp_put_userspace_action(pid, cookie,
-
Re: [ovs-dev] [PATCH] ovsdb-idl: Fix iteration over tracked rows with no actual data.
On 11/23/20 9:37 AM, Ilya Maximets wrote:
> When idl removes orphan rows, those rows are inserted into the
> 'track_list'. This allows iterators such as *_FOR_EACH_TRACKED () to
> return orphan rows that never had any data to the IDL user. In this
> case, it is difficult for the user to understand whether it is a row
> with no data (there was no "insert" / "modify" for this row) or it is
> a row with zero data (columns were cleared by DB transaction).
>
> The main problem with this condition is that rows without data will
> have NULL pointers instead of references that should be there according
> to the database schema. For example, ovn-controller might crash:
>
> ERROR: AddressSanitizer: SEGV on unknown address 0x0100
>(pc 0x0055e9b2 bp 0x7ffef6180880 sp 0x7ffef6180860 T0)
> The signal is caused by a READ memory access.
> Hint: address points to the zero page.
> #0 0x55e9b1 in handle_deleted_lport /controller/binding.c
> #1 0x55e903 in handle_deleted_vif_lport /controller/binding.c:2072:5
> #2 0x55e059 in binding_handle_port_binding_changes
> /controller/binding.c:2155:23
> #3 0x5a6395 in runtime_data_sb_port_binding_handler
> /controller/ovn-controller.c:1454:10
> #4 0x5e15b3 in engine_compute /lib/inc-proc-eng.c:306:18
> #5 0x5e0faf in engine_run_node /lib/inc-proc-eng.c:352:14
> #6 0x5e0e04 in engine_run /lib/inc-proc-eng.c:377:9
> #7 0x5a03de in main /controller/ovn-controller.c
> #8 0x7f4fd9c991a2 in __libc_start_main (/lib64/libc.so.6+0x271a2)
> #9 0x483f0d in _start (/controller/ovn-controller+0x483f0d)
>
> It doesn't make much sense to return non-real rows to the user, so it's
> best to exclude them from iteration.
>
> Test included. Without the fix, provided test will print empty orphan
> rows that was never received by idl as tracked changes.
>
> Fixes: 932104f483ef ("ovsdb-idl: Add support for change tracking.")
> Signed-off-by: Ilya Maximets
> ---
Looks good to me, thanks!
Acked-by: Dumitru Ceara
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 13/14] ovn-nbctl: Fix IP leak on failure of lr policy addition.
On Fri, Nov 20, 2020 at 11:35 PM Dumitru Ceara wrote:
>
> On 11/20/20 6:52 PM, Ilya Maximets wrote:
> > On 11/20/20 6:12 PM, Dumitru Ceara wrote:
> >> On 11/20/20 1:17 AM, Ilya Maximets wrote:
> >>> Fixes: 742474bad730 ("ovn-nbctl: Enhance lr-policy-add to set the
> >>> options.")
> >>> Signed-off-by: Ilya Maximets
> >>> ---
> >>> utilities/ovn-nbctl.c | 1 +
> >>> 1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> >>> index 526dbf86c..448c86f42 100644
> >>> --- a/utilities/ovn-nbctl.c
> >>> +++ b/utilities/ovn-nbctl.c
> >>> @@ -3689,6 +3689,7 @@ nbctl_lr_policy_add(struct ctl_context *ctx)
> >>> } else {
> >>> ctl_error(ctx, "No value specified for the option : %s",
> >>> key);
> >>> free(key);
> >>> +free(next_hop);
> >>
> >> Looks like we also need to:
> >>
> >> smap_destroy();
> >
> > Good catch. We also need to free 'value', I guess, because an empty
> > string still consumes some memory (at least one byte).
> >
> > This might be fixed in a separate patch, though.
> >
> > Dumitru, Numan, what do you think?
>
> Sounds good to me, thanks!
Sounds good to me too.
Numan
>
> ___
> 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
[ovs-dev] [PATCH] ovsdb-idl: Fix iteration over tracked rows with no actual data.
When idl removes orphan rows, those rows are inserted into the
'track_list'. This allows iterators such as *_FOR_EACH_TRACKED () to
return orphan rows that never had any data to the IDL user. In this
case, it is difficult for the user to understand whether it is a row
with no data (there was no "insert" / "modify" for this row) or it is
a row with zero data (columns were cleared by DB transaction).
The main problem with this condition is that rows without data will
have NULL pointers instead of references that should be there according
to the database schema. For example, ovn-controller might crash:
ERROR: AddressSanitizer: SEGV on unknown address 0x0100
(pc 0x0055e9b2 bp 0x7ffef6180880 sp 0x7ffef6180860 T0)
The signal is caused by a READ memory access.
Hint: address points to the zero page.
#0 0x55e9b1 in handle_deleted_lport /controller/binding.c
#1 0x55e903 in handle_deleted_vif_lport /controller/binding.c:2072:5
#2 0x55e059 in binding_handle_port_binding_changes
/controller/binding.c:2155:23
#3 0x5a6395 in runtime_data_sb_port_binding_handler
/controller/ovn-controller.c:1454:10
#4 0x5e15b3 in engine_compute /lib/inc-proc-eng.c:306:18
#5 0x5e0faf in engine_run_node /lib/inc-proc-eng.c:352:14
#6 0x5e0e04 in engine_run /lib/inc-proc-eng.c:377:9
#7 0x5a03de in main /controller/ovn-controller.c
#8 0x7f4fd9c991a2 in __libc_start_main (/lib64/libc.so.6+0x271a2)
#9 0x483f0d in _start (/controller/ovn-controller+0x483f0d)
It doesn't make much sense to return non-real rows to the user, so it's
best to exclude them from iteration.
Test included. Without the fix, provided test will print empty orphan
rows that was never received by idl as tracked changes.
Fixes: 932104f483ef ("ovsdb-idl: Add support for change tracking.")
Signed-off-by: Ilya Maximets
---
lib/ovsdb-idl.c | 22 ++-
tests/idltest.ovsschema | 15 ++
tests/ovsdb-idl.at | 55 +
tests/test-ovsdb.c | 61 +
4 files changed, 140 insertions(+), 13 deletions(-)
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 6334061b4..23648ff6b 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -1892,29 +1892,37 @@ ovsdb_idl_track_is_set(struct ovsdb_idl_table *table)
}
/* Returns the first tracked row in table with class 'table_class'
- * for the specified 'idl'. Returns NULL if there are no tracked rows */
+ * for the specified 'idl'. Returns NULL if there are no tracked rows.
+ * Pure orphan rows, i.e. rows that never had any datum, are skipped. */
const struct ovsdb_idl_row *
ovsdb_idl_track_get_first(const struct ovsdb_idl *idl,
const struct ovsdb_idl_table_class *table_class)
{
struct ovsdb_idl_table *table
= ovsdb_idl_db_table_from_class(>data, table_class);
+struct ovsdb_idl_row *row;
-if (!ovs_list_is_empty(>track_list)) {
-return CONTAINER_OF(ovs_list_front(>track_list), struct
ovsdb_idl_row, track_node);
+LIST_FOR_EACH (row, track_node, >track_list) {
+if (!ovsdb_idl_row_is_orphan(row) || row->tracked_old_datum) {
+return row;
+}
}
return NULL;
}
/* Returns the next tracked row in table after the specified 'row'
- * (in no particular order). Returns NULL if there are no tracked rows */
+ * (in no particular order). Returns NULL if there are no tracked rows.
+ * Pure orphan rows, i.e. rows that never had any datum, are skipped.*/
const struct ovsdb_idl_row *
ovsdb_idl_track_get_next(const struct ovsdb_idl_row *row)
{
-if (row->track_node.next != >table->track_list) {
-return CONTAINER_OF(row->track_node.next, struct ovsdb_idl_row,
track_node);
-}
+struct ovsdb_idl_table *table = row->table;
+LIST_FOR_EACH_CONTINUE (row, track_node, >track_list) {
+if (!ovsdb_idl_row_is_orphan(row) || row->tracked_old_datum) {
+return row;
+}
+}
return NULL;
}
diff --git a/tests/idltest.ovsschema b/tests/idltest.ovsschema
index e04755ea0..3ddb612b0 100644
--- a/tests/idltest.ovsschema
+++ b/tests/idltest.ovsschema
@@ -195,6 +195,21 @@
},
"isRoot": true
},
+"simple6": {
+ "columns" : {
+"name": {"type": "string"},
+"weak_ref": {
+ "type": {
+"key": {"type": "uuid",
+"refTable": "simple",
+"refType": "weak"},
+"min": 0,
+"max": "unlimited"
+ }
+}
+ },
+ "isRoot": true
+},
"singleton" : {
"columns" : {
"name" : {
diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
index cacc82d82..406a57627 100644
--- a/tests/ovsdb-idl.at
+++ b/tests/ovsdb-idl.at
@@ -967,6 +967,7 @@ AT_CHECK([grep ovsdb_idl stderr | sort], [0], [dnl
test-ovsdb|ovsdb_idl|idltest database lacks indexed table (database needs
upgrade?)
test-ovsdb|ovsdb_idl|idltest
