Re: [ovs-dev] [PATCH v5 OVN] ovn-nbctl.c: Add an optional way to delete QoS by uuid
Bleep bloop. Greetings Tao YunXiang, I am a robot and I have tried out your
patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line lacks whitespace around operator
#34 FILE: utilities/ovn-nbctl.c:607:
qos-del SWITCH [{DIRECTION | UUID} [PRIORITY MATCH]]\n\
Lines checked: 98, Warnings: 1, Errors: 0
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 v5 OVN] ovn-nbctl.c: Add an optional way to delete QoS by uuid
We can delete qos by specify ls and more parameters.
If CMS want to delete it exactly, it must specify detailed "match" field.
It's not an easy way, also maybe deleted by mistake.
This change adds a way to specify ls and uuid, which is optional.
You can still use the previous method to delete.
usage:
ovn-nbctl qos-del ls0 [UUID0]
Author: Tao YunXiang
Co-authored-by: Liu Chang
Co-authored-by: Rong Yin
Signed-off-by: Tao YunXiang
Signed-off-by: Liu Chang
Signed-off-by: Rong Yin
---
v4: Add a way to delete QoS by its name or uuid
v3: ovn-nbctl.c: Add a way to delete QoS by its name or uuid
---
utilities/ovn-nbctl.c | 39 ---
1 file changed, 28 insertions(+), 11 deletions(-)
diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index e80058e61..5b2fa6084 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -604,7 +604,7 @@ ACL commands:\n\
QoS commands:\n\
qos-add SWITCH DIRECTION PRIORITY MATCH [rate=RATE [burst=BURST]]
[dscp=DSCP]\n\
add an QoS rule to SWITCH\n\
- qos-del SWITCH [DIRECTION [PRIORITY MATCH]]\n\
+ qos-del SWITCH [{DIRECTION | UUID} [PRIORITY MATCH]]\n\
remove QoS rules from SWITCH\n\
qos-list SWITCH print QoS rules for SWITCH\n\
\n\
@@ -2521,22 +2521,39 @@ nbctl_qos_del(struct ctl_context *ctx)
}
const char *direction;
-error = parse_direction(ctx->argv[2], &direction);
-if (error) {
-ctx->error = error;
-return;
+const struct uuid *qos_rule_uuid = NULL;
+struct uuid uuid_from_cmd;
+if (uuid_from_string(&uuid_from_cmd, ctx->argv[2])) {
+qos_rule_uuid = &uuid_from_cmd;
+} else {
+error = parse_direction(ctx->argv[2], &direction);
+if (error) {
+ctx->error = error;
+return;
+}
}
-/* If priority and match are not specified, delete all qos_rules with the
- * specified direction. */
+/* If uuid was specified, delete qos_rule with the
+ * specified uuid. */
if (ctx->argc == 3) {
struct nbrec_qos **new_qos_rules
= xmalloc(sizeof *new_qos_rules * ls->n_qos_rules);
int n_qos_rules = 0;
-for (size_t i = 0; i < ls->n_qos_rules; i++) {
-if (strcmp(direction, ls->qos_rules[i]->direction)) {
-new_qos_rules[n_qos_rules++] = ls->qos_rules[i];
+if (qos_rule_uuid) {
+for (size_t i = 0; i < ls->n_qos_rules; i++) {
+if (!uuid_equals(qos_rule_uuid,
+ &(ls->qos_rules[i]->header_.uuid))) {
+new_qos_rules[n_qos_rules++] = ls->qos_rules[i];
+}
+}
+/* If priority and match are not specified, delete all qos_rules
+ * with the specified direction. */
+} else {
+for (size_t i = 0; i < ls->n_qos_rules; i++) {
+if (strcmp(direction, ls->qos_rules[i]->direction)) {
+new_qos_rules[n_qos_rules++] = ls->qos_rules[i];
+}
}
}
@@ -6030,7 +6047,7 @@ static const struct ctl_command_syntax nbctl_commands[] =
{
{ "qos-add", 5, 7,
"SWITCH DIRECTION PRIORITY MATCH [rate=RATE [burst=BURST]] [dscp=DSCP]",
NULL, nbctl_qos_add, NULL, "--may-exist", RW },
-{ "qos-del", 1, 4, "SWITCH [DIRECTION [PRIORITY MATCH]]", NULL,
+{ "qos-del", 1, 4, "SWITCH [{DIRECTION | UUID} [PRIORITY MATCH]]", NULL,
nbctl_qos_del, NULL, "", RW },
{ "qos-list", 1, 1, "SWITCH", NULL, nbctl_qos_list, NULL, "", RO },
--
2.17.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] Loan Application Details
Attention, Kindly Find Attached Files And Send Your Documents Back To Us. Apply With Us On Our 5% Interest Rate, We Offer for all categories. Personal, Home, Debt Consolidation And Business Loans. Even thou you are blacklisted or under debt review. Legal Registration No. : 2014/238085/07 Regards, Mrs. Paula Rigt Office Line: +27 679 616 466 Emails: expressl...@webmail.co.za And expressloan2...@outlook.com ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] 答复: 答复: [PATCH v7] Use TPACKET_V3 to accelerate veth for userspace datapath
Hi, folks As I said, TPACKET_V3 does have kernel implementation issue, I tried to fix it in Linux kernel 5.5.9, here is my test data with tpacket_v3 and tso enabled. On my low end server, my goal is to reach 16Gbps at least, I still have another idea to improve it. [yangyi@localhost ovs-master]$ sudo ../run-iperf3.sh Connecting to host 10.15.1.3, port 5201 [ 4] local 10.15.1.2 port 42336 connected to 10.15.1.3 port 5201 [ ID] Interval Transfer Bandwidth Retr Cwnd [ 4] 0.00-10.00 sec 12.9 GBytes 11.1 Gbits/sec1 3.09 MBytes [ 4] 10.00-20.00 sec 12.9 GBytes 11.1 Gbits/sec0 3.09 MBytes [ 4] 20.00-30.00 sec 12.9 GBytes 11.1 Gbits/sec3 3.09 MBytes [ 4] 30.00-40.00 sec 12.8 GBytes 11.0 Gbits/sec0 3.09 MBytes [ 4] 40.00-50.00 sec 12.8 GBytes 11.0 Gbits/sec0 3.09 MBytes [ 4] 50.00-60.00 sec 12.8 GBytes 11.0 Gbits/sec0 3.09 MBytes - - - - - - - - - - - - - - - - - - - - - - - - - [ ID] Interval Transfer Bandwidth Retr [ 4] 0.00-60.00 sec 77.2 GBytes 11.1 Gbits/sec4 sender [ 4] 0.00-60.00 sec 77.2 GBytes 11.1 Gbits/sec receiver Server output: Accepted connection from 10.15.1.2, port 42334 [ 5] local 10.15.1.3 port 5201 connected to 10.15.1.2 port 42336 [ ID] Interval Transfer Bandwidth [ 5] 0.00-10.00 sec 12.9 GBytes 11.1 Gbits/sec [ 5] 10.00-20.00 sec 12.9 GBytes 11.1 Gbits/sec [ 5] 20.00-30.00 sec 12.9 GBytes 11.1 Gbits/sec [ 5] 30.00-40.00 sec 12.8 GBytes 11.0 Gbits/sec [ 5] 40.00-50.00 sec 12.8 GBytes 11.0 Gbits/sec [ 5] 50.00-60.00 sec 12.8 GBytes 11.0 Gbits/sec [ 5] 60.00-60.01 sec 14.3 MBytes 12.4 Gbits/sec - - - - - - - - - - - - - - - - - - - - - - - - - [ ID] Interval Transfer Bandwidth [ 5] 0.00-60.01 sec 0.00 Bytes 0.00 bits/sec sender [ 5] 0.00-60.01 sec 77.2 GBytes 11.0 Gbits/sec receiver iperf Done. [yangyi@localhost ovs-master]$ -邮件原件- 发件人: William Tu [mailto:u9012...@gmail.com] 发送时间: 2020年3月19日 5:42 收件人: Yi Yang (杨燚)-云服务集团 抄送: i.maxim...@ovn.org; yang_y...@163.com; ovs-dev@openvswitch.org 主题: Re: [ovs-dev] 答复: [PATCH v7] Use TPACKET_V3 to accelerate veth for userspace datapath On Wed, Mar 18, 2020 at 6:22 AM Yi Yang (杨燚)-云服务集团 wrote: > > Ilya, raw socket for the interface type of which is "system" has been > set to non-block mode, can you explain which syscall will lead to > sleep? Yes, pmd thread will consume CPU resource even if it has > nothing to do, but all the type=dpdk ports are handled by pmd thread, > here we just let system interfaces look like a DPDK interface. I > didn't see any problem in my test, it will be better if you can tell > me what will result in a problem and how I can reproduce it. By the > way, type=tap/internal interfaces are still be handled by ovs-vswitchd thread. > > In addition, only one line change is there, ".is_pmd = true,", > ".is_pmd = false," will keep it in ovs-vswitchd if there is any other > concern. We can change non-thread-safe parts to support pmd. > Hi Yiyang an Ilya, How about making tpacket_v3 a new netdev class with type="tpacket"? Like my original patch: https://mail.openvswitch.org/pipermail/ovs-dev/2019-December/366229.html Users have to create it specifically by doing type="tpacket", ex: $ ovs-vsctl add-port br0 enp2s0 -- set interface enp2s0 type="tpacket" And we can set is_pmd=true for this particular type. Regards William ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] hmap.h: Fix Coverity false positive
Bleep bloop. Greetings Usman Ansari, 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: corrupt patch at line 17 error: could not build fake ancestor hint: Use 'git am --show-current-patch' to see the failed patch Patch failed at 0001 hmap.h: Fix Coverity false positive When you have resolved this problem, run "git am --continue". 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] hmap.h: Fix Coverity false positive
On Wed, Mar 18, 2020 at 05:32:03PM -0700, Usman Ansari wrote: > Suggested-by: Ben Pfaff > > Coverity reports a false positive below: > Incorrect expression, Assign_where_compare_meant: use of "=" > where "==" may have been intended. > Fixed it by rewriting '(NODE = NULL)' as '((NODE = NULL), false)'. > "make check" passes for this change > Coverity reports 80 errors resolved > > Signed-off-by: Usman Ansari Thanks. This doesn't apply. "git am" says: Applying: hmap.h: Fix Coverity false positive error: corrupt patch at line 14 Patch failed at 0001 hmap.h: Fix Coverity false positive hint: Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Also, all tags go at the bottom of the commit message, that is, Suggested-by is in the wrong place. The committer documentation has some more rules for tags. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] hmap.h: Fix Coverity false positive
Suggested-by: Ben Pfaff Coverity reports a false positive below: Incorrect expression, Assign_where_compare_meant: use of "=" where "==" may have been intended. Fixed it by rewriting '(NODE = NULL)' as '((NODE = NULL), false)'. "make check" passes for this change Coverity reports 80 errors resolved Signed-off-by: Usman Ansari --- include/openvswitch/hmap.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/openvswitch/hmap.h b/include/openvswitch/hmap.h index 8aea9c2..f48359f 100644 --- a/include/openvswitch/hmap.h +++ b/include/openvswitch/hmap.h @@ -136,7 +136,7 @@ struct hmap_node *hmap_random_node(const struct hmap *); */ #define HMAP_FOR_EACH_WITH_HASH(NODE, MEMBER, HASH, HMAP) \ for (INIT_CONTAINER(NODE, hmap_first_with_hash(HMAP, HASH), MEMBER); \ - (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE = NULL); \ + (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || ((NODE = NULL), false); \ ASSIGN_CONTAINER(NODE, hmap_next_with_hash(&(NODE)->MEMBER), \ MEMBER)) #define HMAP_FOR_EACH_IN_BUCKET(NODE, MEMBER, HASH, HMAP) \ -- 2.7.4 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] 答复: 答复: [PATCH v7] Use TPACKET_V3 to accelerate veth for userspace datapath
William, that can't fix Ilya's concern, we can fix thread-safe issues if most of code in lib/netdev-linux.c is not thread-safe, we also have to consider how we can fix scalability issue, obviously it isn't scalable that one ovs-vswitchd thread handles all the such interfaces, this is performance bottleneck and the performance is linearly related with number of interfaces. I think pmd thread is our natural choice, we have no reason to refuse this way, we can first fix them if current code does have issues to support pmd thread. If it is difficult to support pmd currently, I can remove ".is_pmd = true", if a user wants to do that way, maybe we can add an option in interface level, options:is_pmd=true, but I'm not sure if is_pmd can be set on adding interface. -邮件原件- 发件人: William Tu [mailto:u9012...@gmail.com] 发送时间: 2020年3月19日 5:42 收件人: Yi Yang (杨燚)-云服务集团 抄送: i.maxim...@ovn.org; yang_y...@163.com; ovs-dev@openvswitch.org 主题: Re: [ovs-dev] 答复: [PATCH v7] Use TPACKET_V3 to accelerate veth for userspace datapath On Wed, Mar 18, 2020 at 6:22 AM Yi Yang (杨燚)-云服务集团 wrote: > > Ilya, raw socket for the interface type of which is "system" has been > set to non-block mode, can you explain which syscall will lead to > sleep? Yes, pmd thread will consume CPU resource even if it has > nothing to do, but all the type=dpdk ports are handled by pmd thread, > here we just let system interfaces look like a DPDK interface. I > didn't see any problem in my test, it will be better if you can tell > me what will result in a problem and how I can reproduce it. By the > way, type=tap/internal interfaces are still be handled by ovs-vswitchd thread. > > In addition, only one line change is there, ".is_pmd = true,", > ".is_pmd = false," will keep it in ovs-vswitchd if there is any other > concern. We can change non-thread-safe parts to support pmd. > Hi Yiyang an Ilya, How about making tpacket_v3 a new netdev class with type="tpacket"? Like my original patch: https://mail.openvswitch.org/pipermail/ovs-dev/2019-December/366229.html Users have to create it specifically by doing type="tpacket", ex: $ ovs-vsctl add-port br0 enp2s0 -- set interface enp2s0 type="tpacket" And we can set is_pmd=true for this particular type. Regards William ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] Taller de Relaciones Laborales
Buen día El cupo del curso está por llenarse y quise aprovechar la oportunidad de hacerte una última invitación: •Nombre: Taller de Relaciones Laborales para mandos medios. (Supervisión efectiva para mandos medios bajo la normatividad laboral vigente) •¿Cuándo?: Miércoles 25 de marzo •Formato: En línea con interacción en vivo. •Lugar: En Vivo desde su computadora La idea del curso es revisar la normatividad vigente en material laboral que está relacionada al liderazgo en mando medios y ofrece herramientas para efectuar este liderazgo de manera efectiva. El participante conocerá las acciones básicas con los lineamientos legales para facilitar su gestión de administración del personal a su cargo en los diferentes escenarios tanto en plantas de manufactura como en empresas de distribución comercial. Proporcionaremos las herramientas más importantes y de vanguardia que se están utilizando para medir la satisfacción de los clientes y que nos ayudarán a centrar la estrategia de nuestra organización en las necesidades de nuestro cliente. Solicita información respondiendo a este correo con la palabra Mandos, junto con los siguientes datos: Nombre: Correo electrónico: Número telefónico: Números de Atención: (045) 55 15 54 66 30 - (045) 55 85 56 72 93 - (045) 55 30 16 70 85 Qué tengas un gran día. Saludos. Si desea dejar de recibir nuestra promoción favor de responder con la palabra baja o enviar un correo a bajas @innovalearn.net ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] 答复: [PATCH v7] Use TPACKET_V3 to accelerate veth for userspace datapath
On Wed, Mar 18, 2020 at 6:22 AM Yi Yang (杨燚)-云服务集团 wrote: > > Ilya, raw socket for the interface type of which is "system" has been set to > non-block mode, can you explain which syscall will lead to sleep? Yes, pmd > thread will consume CPU resource even if it has nothing to do, but all the > type=dpdk ports are handled by pmd thread, here we just let system > interfaces look like a DPDK interface. I didn't see any problem in my test, > it will be better if you can tell me what will result in a problem and how I > can reproduce it. By the way, type=tap/internal interfaces are still be > handled by ovs-vswitchd thread. > > In addition, only one line change is there, ".is_pmd = true,", ".is_pmd = > false," will keep it in ovs-vswitchd if there is any other concern. We can > change non-thread-safe parts to support pmd. > Hi Yiyang an Ilya, How about making tpacket_v3 a new netdev class with type="tpacket"? Like my original patch: https://mail.openvswitch.org/pipermail/ovs-dev/2019-December/366229.html Users have to create it specifically by doing type="tpacket", ex: $ ovs-vsctl add-port br0 enp2s0 -- set interface enp2s0 type="tpacket" And we can set is_pmd=true for this particular type. Regards William ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] bugtool: Fix for Python3
On Wed, Mar 18, 2020 at 8:23 AM Timothy Redaelli wrote: > > Currently ovs-bugtool tool doesn't start on Python3. > This commit fixes ovs-bugtool to make it works on Python.3 > > Reported-at: https://bugzilla.redhat.com/1809241 > Reported-by: Flavio Leitner > Signed-off-by: Timothy Redaelli > --- Thanks for the patch. I tried to fix it before but haven't get it done yet. I run your patch and got this error, can you take a look? ovs# ./utilities/bugtool/ovs-bugtool -y -s --output=tar.gz --outfile=/var/log/bugtool-report.tgz Traceback (most recent call last): File "./utilities/bugtool/ovs-bugtool", line 1405, in sys.exit(main()) File "./utilities/bugtool/ovs-bugtool", line 717, in main collect_data() File "./utilities/bugtool/ovs-bugtool", line 366, in collect_data run_procs(process_lists.values()) File "./utilities/bugtool/ovs-bugtool", line 1344, in run_procs p.read_line() File "./utilities/bugtool/ovs-bugtool", line 1313, in read_line self.inst.write(line.decode()) UnicodeDecodeError: 'utf-8' codec can't decode byte 0xcb in position 67: invalid continuation byte Thanks William ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netdev: Fix integer handling issue.
On Wed, Mar 18, 2020 at 8:36 AM Ben Pfaff wrote:
>
> On Wed, Mar 18, 2020 at 08:34:16AM -0700, William Tu wrote:
> > On Wed, Mar 18, 2020 at 7:59 AM Ilya Maximets wrote:
> > >
> > > On 3/18/20 12:31 AM, William Tu wrote:
> > > > Coverity CID 279497 reports "Operands don't affect result".
> > > > Because flow->ct_state is uint8_t and DP_NETDEV_CS_UNSUPPORTED_MASK
> > > > is '0xff00'. So remove the statement.
> > > >
> > > > Cc: Usman Ansari
> > > > Signed-off-by: William Tu
> > > > ---
> > > > lib/dpif-netdev.c | 4
> > > > 1 file changed, 4 deletions(-)
> > > >
> > > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > > > index a798db45d9cb..0e2678d002d5 100644
> > > > --- a/lib/dpif-netdev.c
> > > > +++ b/lib/dpif-netdev.c
> > > > @@ -3224,10 +3224,6 @@ dpif_netdev_flow_from_nlattrs(const struct
> > > > nlattr *key, uint32_t key_len,
> > > > return EINVAL;
> > > > }
> > > >
> > > > -if (flow->ct_state & DP_NETDEV_CS_UNSUPPORTED_MASK) {
> > > > -return EINVAL;
> > > > -}
> > > > -
> > >
> > > I'm not sure if we need to remove this. This code doesn't make any harm
> > > and most likely compiled out. I agree that it doesn't change any logic
> > > in this function, but in case someone will try to add new flags or change
> > > the type of ct_state we will be safe and will reject all the unknown
> > > flags.
> > > Without this code we'll have to catch this case somehow on code review and
> > > re-introduce this check or implement missing functionality.
> > >
> > > One more thing is that DP_NETDEV_CS_UNSUPPORTED_MASK definition becomes
> > > unused and should be removed along with _SUPPORTED_MASK.
> >
> > Good point.
> >
> > >
> > > So, I'd rather not touch this and just mark this code as OK for coverity
> > > scanner. But if you want to remove, please, clean up other parts and
> > > add a build assert for the ct_state size and flags, so any disruptive
> > > change
> > > will be caught by the developer of this change.
> > >
> > OK thanks!
> > Let's keep this code block as it is now.
>
> I was surprised to hear that it doesn't have any effect. Adding a
> comment might be helpful.
OK, then let me just add a comment.
In case in the future people run Coverity and see this again.
William
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] hmap.h: Fix incorrect expression
I am only sending patch for the macro, for other I will send additional
patch(s)
On Wed, Mar 18, 2020 at 12:33 PM Usman Ansari wrote:
> Will resubmit the patch.
>
> Coverity says 80 bugs fixed from this change.
>
> On Wed, Mar 18, 2020 at 12:19 PM William Tu wrote:
>
> > On Wed, Mar 18, 2020 at 11:55 AM Ben Pfaff wrote:
> > >
> > > On Wed, Mar 18, 2020 at 11:43:29AM -0700, Usman Ansari wrote:
> > > > From Ben Pfaff b...@ovn.org
> > > >
> > > >
> > > > Coverity reports incorrect expression for HMAP_FOR_EACH_WITH_HASH
> macro
> > > > This patch fixes this issue
> > > > "make check" passes for this change
> > > > Coverity reports 80 errors resolved
> > > >
> > > > Signed-off-by: Usman Ansari
> > >
> > > Would you please rephrase the title of the patch? There is nothing
> > > incorrect about the expression, it is just that Coverity complains
> about
> > > it.
> > >
> > > Thanks,
> > >
> > > Ben.
> >
> > Hi Usman,
> >
> > How about change the title to "hmap: Fix Coverity false positive."?
> >
> > Coverity reports a false positive below:
> > Incorrect expression, Assign_where_compare_meant: use of "="
> > where "==" may have been intended.
> > Fixed it by rewriting '(NODE = NULL)' as '((NODE = NULL), false)'.
> >
> > BTW, there are not only this place needed to fix. Please see diff below.
> > With this fix, you should see it fixes around 500 coverity defects.
> >
> > --- a/include/openvswitch/hmap.h
> > +++ b/include/openvswitch/hmap.h
> > @@ -136,12 +136,14 @@ struct hmap_node *hmap_random_node(const struct
> hmap
> > *);
> > */
> > #define HMAP_FOR_EACH_WITH_HASH(NODE, MEMBER, HASH, HMAP)
> \
> > for (INIT_CONTAINER(NODE, hmap_first_with_hash(HMAP, HASH),
> MEMBER); \
> > - (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE =
> > NULL); \
> > + (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
> > + ((NODE = NULL), false); \
> > ASSIGN_CONTAINER(NODE, hmap_next_with_hash(&(NODE)->MEMBER),
> \
> >MEMBER))
> > #define HMAP_FOR_EACH_IN_BUCKET(NODE, MEMBER, HASH, HMAP)
> \
> > for (INIT_CONTAINER(NODE, hmap_first_in_bucket(HMAP, HASH),
> MEMBER); \
> > - (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE =
> > NULL); \
> > + (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
> > + ((NODE = NULL), false); \
> > ASSIGN_CONTAINER(NODE, hmap_next_in_bucket(&(NODE)->MEMBER),
> > MEMBER))
> >
> > static inline struct hmap_node *hmap_first_with_hash(const struct hmap
> *,
> > @@ -170,7 +172,8 @@ bool hmap_contains(const struct hmap *, const
> > struct hmap_node *);
> > HMAP_FOR_EACH_INIT(NODE, MEMBER, HMAP, (void) 0)
> > #define HMAP_FOR_EACH_INIT(NODE, MEMBER, HMAP, ...)
> \
> > for (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER), __VA_ARGS__;
> \
> > - (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE =
> > NULL); \
> > + (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
> > + ((NODE = NULL), false); \
> > ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER),
> MEMBER))
> >
> > /* Safe when NODE may be freed (not needed when NODE may be removed from
> > the
> > @@ -179,7 +182,8 @@ bool hmap_contains(const struct hmap *, const
> > struct hmap_node *);
> > HMAP_FOR_EACH_SAFE_INIT(NODE, NEXT, MEMBER, HMAP, (void) 0)
> > #define HMAP_FOR_EACH_SAFE_INIT(NODE, NEXT, MEMBER, HMAP, ...)
> \
> > for (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER), __VA_ARGS__;
> \
> > - ((NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE =
> > NULL) \
> > + ((NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
> > + ((NODE = NULL), false) \
> >? INIT_CONTAINER(NEXT, hmap_next(HMAP, &(NODE)->MEMBER),
> > MEMBER), 1 \
> >: 0);
> \
> > (NODE) = (NEXT))
> > @@ -190,7 +194,8 @@ bool hmap_contains(const struct hmap *, const
> > struct hmap_node *);
> > #define HMAP_FOR_EACH_CONTINUE_INIT(NODE, MEMBER, HMAP, ...)
> \
> > for (ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER),
> > MEMBER), \
> > __VA_ARGS__;
> \
> > - (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE =
> > NULL); \
> > + (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
> > + ((NODE = NULL), false); \
> > ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER),
> MEMBER))
> >
> > static inline struct hmap_node *
> > @@ -211,7 +216,8 @@ hmap_pop_helper__(struct hmap *hmap, size_t *bucket)
> {
> > #define HMAP_FOR_EACH_POP(NODE, MEMBER, HMAP)
> >\
> > for (size_t bucket__ = 0;
> >\
> > INIT_CONTAINER(NODE, hmap_pop_helper__(HMAP, &bucket__),
> > MEMBER), \
> > - (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE =
> > NULL);)
> > + (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
> > + ((NODE = NULL), false);)
> >
> > static inline struct hmap_node *hmap_first(const struct hm
Re: [ovs-dev] [PATCH] hmap.h: Fix incorrect expression
Will resubmit the patch.
Coverity says 80 bugs fixed from this change.
On Wed, Mar 18, 2020 at 12:19 PM William Tu wrote:
> On Wed, Mar 18, 2020 at 11:55 AM Ben Pfaff wrote:
> >
> > On Wed, Mar 18, 2020 at 11:43:29AM -0700, Usman Ansari wrote:
> > > From Ben Pfaff b...@ovn.org
> > >
> > >
> > > Coverity reports incorrect expression for HMAP_FOR_EACH_WITH_HASH macro
> > > This patch fixes this issue
> > > "make check" passes for this change
> > > Coverity reports 80 errors resolved
> > >
> > > Signed-off-by: Usman Ansari
> >
> > Would you please rephrase the title of the patch? There is nothing
> > incorrect about the expression, it is just that Coverity complains about
> > it.
> >
> > Thanks,
> >
> > Ben.
>
> Hi Usman,
>
> How about change the title to "hmap: Fix Coverity false positive."?
>
> Coverity reports a false positive below:
> Incorrect expression, Assign_where_compare_meant: use of "="
> where "==" may have been intended.
> Fixed it by rewriting '(NODE = NULL)' as '((NODE = NULL), false)'.
>
> BTW, there are not only this place needed to fix. Please see diff below.
> With this fix, you should see it fixes around 500 coverity defects.
>
> --- a/include/openvswitch/hmap.h
> +++ b/include/openvswitch/hmap.h
> @@ -136,12 +136,14 @@ struct hmap_node *hmap_random_node(const struct hmap
> *);
> */
> #define HMAP_FOR_EACH_WITH_HASH(NODE, MEMBER, HASH, HMAP) \
> for (INIT_CONTAINER(NODE, hmap_first_with_hash(HMAP, HASH), MEMBER); \
> - (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE =
> NULL); \
> + (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
> + ((NODE = NULL), false); \
> ASSIGN_CONTAINER(NODE, hmap_next_with_hash(&(NODE)->MEMBER), \
>MEMBER))
> #define HMAP_FOR_EACH_IN_BUCKET(NODE, MEMBER, HASH, HMAP) \
> for (INIT_CONTAINER(NODE, hmap_first_in_bucket(HMAP, HASH), MEMBER); \
> - (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE =
> NULL); \
> + (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
> + ((NODE = NULL), false); \
> ASSIGN_CONTAINER(NODE, hmap_next_in_bucket(&(NODE)->MEMBER),
> MEMBER))
>
> static inline struct hmap_node *hmap_first_with_hash(const struct hmap *,
> @@ -170,7 +172,8 @@ bool hmap_contains(const struct hmap *, const
> struct hmap_node *);
> HMAP_FOR_EACH_INIT(NODE, MEMBER, HMAP, (void) 0)
> #define HMAP_FOR_EACH_INIT(NODE, MEMBER, HMAP, ...) \
> for (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER), __VA_ARGS__; \
> - (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE =
> NULL); \
> + (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
> + ((NODE = NULL), false); \
> ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER))
>
> /* Safe when NODE may be freed (not needed when NODE may be removed from
> the
> @@ -179,7 +182,8 @@ bool hmap_contains(const struct hmap *, const
> struct hmap_node *);
> HMAP_FOR_EACH_SAFE_INIT(NODE, NEXT, MEMBER, HMAP, (void) 0)
> #define HMAP_FOR_EACH_SAFE_INIT(NODE, NEXT, MEMBER, HMAP, ...) \
> for (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER), __VA_ARGS__; \
> - ((NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE =
> NULL) \
> + ((NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
> + ((NODE = NULL), false) \
>? INIT_CONTAINER(NEXT, hmap_next(HMAP, &(NODE)->MEMBER),
> MEMBER), 1 \
>: 0); \
> (NODE) = (NEXT))
> @@ -190,7 +194,8 @@ bool hmap_contains(const struct hmap *, const
> struct hmap_node *);
> #define HMAP_FOR_EACH_CONTINUE_INIT(NODE, MEMBER, HMAP, ...)\
> for (ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER),
> MEMBER), \
> __VA_ARGS__; \
> - (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE =
> NULL); \
> + (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
> + ((NODE = NULL), false); \
> ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER))
>
> static inline struct hmap_node *
> @@ -211,7 +216,8 @@ hmap_pop_helper__(struct hmap *hmap, size_t *bucket) {
> #define HMAP_FOR_EACH_POP(NODE, MEMBER, HMAP)
>\
> for (size_t bucket__ = 0;
>\
> INIT_CONTAINER(NODE, hmap_pop_helper__(HMAP, &bucket__),
> MEMBER), \
> - (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE =
> NULL);)
> + (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
> + ((NODE = NULL), false);)
>
> static inline struct hmap_node *hmap_first(const struct hmap *);
> static inline struct hmap_node *hmap_next(const struct hmap *,
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] Consulta
Consulta ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] hmap.h: Fix incorrect expression
On Wed, Mar 18, 2020 at 11:55 AM Ben Pfaff wrote:
>
> On Wed, Mar 18, 2020 at 11:43:29AM -0700, Usman Ansari wrote:
> > From Ben Pfaff b...@ovn.org
> >
> >
> > Coverity reports incorrect expression for HMAP_FOR_EACH_WITH_HASH macro
> > This patch fixes this issue
> > "make check" passes for this change
> > Coverity reports 80 errors resolved
> >
> > Signed-off-by: Usman Ansari
>
> Would you please rephrase the title of the patch? There is nothing
> incorrect about the expression, it is just that Coverity complains about
> it.
>
> Thanks,
>
> Ben.
Hi Usman,
How about change the title to "hmap: Fix Coverity false positive."?
Coverity reports a false positive below:
Incorrect expression, Assign_where_compare_meant: use of "="
where "==" may have been intended.
Fixed it by rewriting '(NODE = NULL)' as '((NODE = NULL), false)'.
BTW, there are not only this place needed to fix. Please see diff below.
With this fix, you should see it fixes around 500 coverity defects.
--- a/include/openvswitch/hmap.h
+++ b/include/openvswitch/hmap.h
@@ -136,12 +136,14 @@ struct hmap_node *hmap_random_node(const struct hmap *);
*/
#define HMAP_FOR_EACH_WITH_HASH(NODE, MEMBER, HASH, HMAP) \
for (INIT_CONTAINER(NODE, hmap_first_with_hash(HMAP, HASH), MEMBER); \
- (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE = NULL); \
+ (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
+ ((NODE = NULL), false); \
ASSIGN_CONTAINER(NODE, hmap_next_with_hash(&(NODE)->MEMBER), \
MEMBER))
#define HMAP_FOR_EACH_IN_BUCKET(NODE, MEMBER, HASH, HMAP) \
for (INIT_CONTAINER(NODE, hmap_first_in_bucket(HMAP, HASH), MEMBER); \
- (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE = NULL); \
+ (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
+ ((NODE = NULL), false); \
ASSIGN_CONTAINER(NODE, hmap_next_in_bucket(&(NODE)->MEMBER), MEMBER))
static inline struct hmap_node *hmap_first_with_hash(const struct hmap *,
@@ -170,7 +172,8 @@ bool hmap_contains(const struct hmap *, const
struct hmap_node *);
HMAP_FOR_EACH_INIT(NODE, MEMBER, HMAP, (void) 0)
#define HMAP_FOR_EACH_INIT(NODE, MEMBER, HMAP, ...) \
for (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER), __VA_ARGS__; \
- (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE = NULL); \
+ (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
+ ((NODE = NULL), false); \
ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER))
/* Safe when NODE may be freed (not needed when NODE may be removed from the
@@ -179,7 +182,8 @@ bool hmap_contains(const struct hmap *, const
struct hmap_node *);
HMAP_FOR_EACH_SAFE_INIT(NODE, NEXT, MEMBER, HMAP, (void) 0)
#define HMAP_FOR_EACH_SAFE_INIT(NODE, NEXT, MEMBER, HMAP, ...) \
for (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER), __VA_ARGS__; \
- ((NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE = NULL) \
+ ((NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
+ ((NODE = NULL), false) \
? INIT_CONTAINER(NEXT, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER), 1 \
: 0); \
(NODE) = (NEXT))
@@ -190,7 +194,8 @@ bool hmap_contains(const struct hmap *, const
struct hmap_node *);
#define HMAP_FOR_EACH_CONTINUE_INIT(NODE, MEMBER, HMAP, ...)\
for (ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER), \
__VA_ARGS__; \
- (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE = NULL); \
+ (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
+ ((NODE = NULL), false); \
ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER))
static inline struct hmap_node *
@@ -211,7 +216,8 @@ hmap_pop_helper__(struct hmap *hmap, size_t *bucket) {
#define HMAP_FOR_EACH_POP(NODE, MEMBER, HMAP) \
for (size_t bucket__ = 0; \
INIT_CONTAINER(NODE, hmap_pop_helper__(HMAP, &bucket__), MEMBER), \
- (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE = NULL);)
+ (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || \
+ ((NODE = NULL), false);)
static inline struct hmap_node *hmap_first(const struct hmap *);
static inline struct hmap_node *hmap_next(const struct hmap *,
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2 3/7] ovn-nb: Better document distributed gateway ports.
Bleep bloop. Greetings Ben Pfaff, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line is 83 characters long (recommended limit is 79)
#218 FILE: ovn-nb.xml:2160:
type='{"type": "string", "enum": ["set", ["overlay",
"bridged"]]}'>
Lines checked: 337, Warnings: 1, Errors: 0
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] Coverity incorrect expression of HMAP_FOR_EACH_WITH_HASH
Ben,
I have verified your change to the macro and sending the patch.
Thanks.
On 3/17/20, 8:33 PM, "dev on behalf of William Tu"
wrote:
On Tue, Mar 17, 2020 at 5:39 PM Ben Pfaff wrote:
>
> On Tue, Mar 17, 2020 at 05:00:42PM -0700, William Tu wrote:
> > Hi,
> >
> > We're looking at the coverity results and there are lots of issues of
> > Incorrect expression
> > Assign_where_compare_meant: use of "=" where "==" may have been intended
> >
> > Because in our source code, we have
> > HMAP_FOR_EACH_WITH_HASH(id_node, node, hash, &pool->map) {
> > ...
> >
> > And Due to "(NODE = NULL)" in the CONDITION statement below:
> > #define HMAP_FOR_EACH_WITH_HASH(NODE, MEMBER, HASH, HMAP)
\
> > for (INIT_CONTAINER(NODE, hmap_first_with_hash(HMAP, HASH),
MEMBER); \
> > (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE =
NULL); \
> > ASSIGN_CONTAINER(NODE, hmap_next_with_hash(&(NODE)->MEMBER),
\
> > MEMBER))
> >
> > While this is correct, I wonder how can we avoid this?
>
> We could rewrite (NODE = NULL) as ((NODE = NULL), false).
Thanks!
Let me try it and get back to you.
William
___
dev mailing list
d...@openvswitch.org
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=02%7C01%7Cuansari%40vmware.com%7C8948d58fbeac4ddf83b908d7caed2402%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637200992195133490&sdata=o2Ini421ztOl84NHRgKzD2fUkMo1BQGoGpNXeGn%2Fg%2Bk%3D&reserved=0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] hmap.h: Fix incorrect expression
On Wed, Mar 18, 2020 at 11:43:29AM -0700, Usman Ansari wrote: > From Ben Pfaff b...@ovn.org > > > Coverity reports incorrect expression for HMAP_FOR_EACH_WITH_HASH macro > This patch fixes this issue > "make check" passes for this change > Coverity reports 80 errors resolved > > Signed-off-by: Usman Ansari Would you please rephrase the title of the patch? There is nothing incorrect about the expression, it is just that Coverity complains about it. Thanks, Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v2 5/7] ovn-architecture: Don't imply the wrong thing about NAT.
Suggested-by: Han Zhou Signed-off-by: Ben Pfaff --- ovn-architecture.7.xml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ovn-architecture.7.xml b/ovn-architecture.7.xml index a69f4af7d1c5..b88bba5a1be9 100644 --- a/ovn-architecture.7.xml +++ b/ovn-architecture.7.xml @@ -636,7 +636,8 @@ DNAT and SNAT rules may be associated with a gateway router, which provides a central location that can handle one-to-many SNAT (aka IP -masquerading). +masquerading). Distributed gateway ports, described below, also +support NAT. Distributed Gateway Ports -- 2.24.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v2 6/7] ovn-architecture: Distributed gateway ports can interconnect OVNs.
Also, reduce redundancy. Suggested-by: Han Zhou Signed-off-by: Ben Pfaff --- ovn-architecture.7.xml | 29 +++-- 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/ovn-architecture.7.xml b/ovn-architecture.7.xml index b88bba5a1be9..a250cb38d811 100644 --- a/ovn-architecture.7.xml +++ b/ovn-architecture.7.xml @@ -643,14 +643,15 @@ Distributed Gateway Ports -A distributed gateway port is a logical router port -that is specially configured to designate one distinguished -chassis, called the gateway chassis, for centralized -processing. A distributed gateway port should connect to a -logical switch with a localnet port. Packets to and -from the distributed gateway are processed without involving the -gateway chassis when they can be, but when needed they do take -an extra hop through it. +A distributed gateway port is a logical router port that is +specially configured to designate one distinguished chassis, called the +gateway chassis, for centralized processing. A distributed +gateway port should connect to a logical switch that has an LSP that +connects externally, that is, either a localnet LSP or a +connection to another OVN deployment (see OVN Deployments +Interconnection). Packets that traverse the distributed gateway +port are processed without involving the gateway chassis when they can be, +but when needed they do take an extra hop through it. @@ -1581,16 +1582,8 @@ Distributed Gateway Ports -Distributed gateway ports are logical router patch ports -that directly connect distributed logical routers to logical -switches with external connection. - - - -There are two types of external connections. Firstly, connection to -physical network through a localnet port. Secondly, connection to -another OVN deployment, which will be introduced in section "OVN -Deployments Interconnection". +This section provides additional details on distributed gateway ports, +outlined earlier. -- 2.24.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v2 7/7] docs: Acknowledge in more places that localnet isn't the only option.
Signed-off-by: Ben Pfaff --- ovn-architecture.7.xml | 10 +- ovn-nb.xml | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/ovn-architecture.7.xml b/ovn-architecture.7.xml index a250cb38d811..533ae716d11d 100644 --- a/ovn-architecture.7.xml +++ b/ovn-architecture.7.xml @@ -1592,11 +1592,11 @@ where a VM or container resides. Whenever possible, packets from the VM or container to the outside world should be processed completely on that VM's or container's hypervisor, eventually -traversing a localnet port instance on that hypervisor to the -physical network. Whenever possible, packets from the outside -world to a VM or container should be directed through the physical -network directly to the VM's or container's hypervisor, where the -packet will enter the integration bridge through a localnet port. +traversing a localnet port instance or a tunnel to the physical +network or a different OVN deployment. Whenever possible, packets +from the outside world to a VM or container should be directed +through the physical network directly to the VM's or container's +hypervisor. diff --git a/ovn-nb.xml b/ovn-nb.xml index 7f142bd35031..108889713182 100644 --- a/ovn-nb.xml +++ b/ovn-nb.xml @@ -2038,9 +2038,9 @@ If any of these are set, this logical router port represents a distributed gateway port that connects this router to a -logical switch with a localnet port. There may -be at most one such logical router port on each logical -router. +logical switch with a localnet port or a +connection to another OVN deployment. There may be at most +one such logical router port on each logical router. -- 2.24.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v2 3/7] ovn-nb: Better document distributed gateway ports.
The documentation was scattered across two places. This brings it together. It also documents the two solutions for physical VLAN MTU issues in a unified way. Signed-off-by: Ben Pfaff --- ovn-architecture.7.xml | 110 +-- ovn-nb.xml | 166 - 2 files changed, 181 insertions(+), 95 deletions(-) diff --git a/ovn-architecture.7.xml b/ovn-architecture.7.xml index d32978a9d5be..94a897fa693e 100644 --- a/ovn-architecture.7.xml +++ b/ovn-architecture.7.xml @@ -688,6 +688,100 @@ highest-priority gateway that is online. + Physical VLAN MTU Issues + + +Consider the preceding diagram again: + + + +LSlocal + | + LR1 + | + +++ + ||| + LS1 ... LSn + + + +Suppose that each logical switch LS1, ..., LSn is bridged to a physical +VLAN-tagged network attached to a localnet port on LSlocal, +over a distributed gateway port on LR1. If a packet originating on +LSi is destined to the external network, OVN sends it to the +gateway chassis over a tunnel. There, the packet traverses LR1's logical +router pipeline, possibly undergoes NAT, and eventually ends up at +LSlocal's localnet port. If all of the physical links in the +network have the same MTU, then the packet's transit across a tunnel causes +an MTU problem: tunnel overhead prevents a packet that uses the full +physical MTU from crossing the tunnel to the gateway chassis (without +fragmentation). + + + +OVN offers two solutions to this problem, the +reside-on-redirect-chassis and redirect-type +options. Both solutions require each logical switch LS1, ..., LSn to +include a localnet logical switch port LN1, ..., LNn +respectively, that is present on each chassis. Both cause packets to be +sent over the localnet ports instead of tunnels. They differ +in which packets--some or all--are sent this way. The most prominent +tradeoff between these options is that +reside-on-redirect-chassis is easier to configure and that +redirect-type performs better for east-west traffic. + + + +The first solution is the reside-on-redirect-chassis option +for logical router ports. Setting this option on a LRP from (e.g.) LS1 to +LR1 disables forwarding from LS1 to LR1 except on the gateway chassis. On +chassis other than the gateway chassis, this single change means that +packets that would otherwise have been forwarded to LR1 are instead +forwarded to LN1. The instance of LN1 on the gateway chassis then receives +the packet and forwards it to LR1. The packet traverses the LR1 logical +router pipeline, possibly undergoes NAT, and eventually ends up at +LSlocal's localnet port. The packet never traverses a tunnel, +avoiding the MTU issue. + + + +This option has the further consequence of centralizing ``distributed'' +logical router LR1, since no packets are forwarded from LS1 to LR1 on any +chassis other than the gateway chassis. Therefore, east-west traffic +passes through the gateway chassis, not just north-south. (The naive +``fix'' of allowing east-west traffic to flow directly between chassis over +LN1 does not work because routing sets the Ethernet source address to LR1's +source address. Seeing this single Ethernet source address originate from +all of the chassis will confuse the physical switch.) + + + +Do not set the reside-on-redirect-chassis option on a +distributed gateway port. In the diagram above, it would be set on the +LRPs connecting LS1, ..., LSn to LR1. + + + +The second solution is the redirect-chassis option for +distributed gateway ports. Setting this option to bridged +causes packets that are redirected to the gateway chassis to go over the +localnet ports instead of being tunneled. This option does +not change how OVN treats packets not redirected to the gateway chassis. + + + +The redirect-chassis option requires the administrator or the +CMS to configure each participating chassis with a unique Ethernet address +for the locgical router by setting ovn-chassis-mac-mappings in +the Open vSwitch database, for use by ovn-controller. This +makes it more difficult to configure than +reside-on-redirect-chassis. + + + +Set the redirect-chassis option on a distributed gateway port. + + Life Cycle of a VIF @@ -1892,14 +1986,14 @@ -VLAN-based redirection: - -As an enhancement to reside-on-redirect-chassis we support -VLAN-based redirection as well. By setting -options:redirect-type to bridged on a gateway -chassis attached router port, user can
[ovs-dev] [PATCH ovn v2 0/7] gateway documentation improvements
v1->v2: Fix problems pointed out by Numan and Han. Ben Pfaff (7): xml2nroff: Properly support . ovn-architecture: Flip network diagrams. ovn-nb: Better document distributed gateway ports. ovn-architecture: Correct documentation of localnet ports. ovn-architecture: Don't imply the wrong thing about NAT. ovn-architecture: Distributed gateway ports can interconnect OVNs. docs: Acknowledge in more places that localnet isn't the only option. build-aux/xml2nroff| 10 +- ovn-architecture.7.xml | 201 ++--- ovn-nb.xml | 166 -- 3 files changed, 237 insertions(+), 140 deletions(-) -- 2.24.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v2 4/7] ovn-architecture: Correct documentation of localnet ports.
Their LSes can have multiple additional LSPs, not just one. Suggested-by: Han Zhou Signed-off-by: Ben Pfaff --- ovn-architecture.7.xml | 29 - 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/ovn-architecture.7.xml b/ovn-architecture.7.xml index 94a897fa693e..a69f4af7d1c5 100644 --- a/ovn-architecture.7.xml +++ b/ovn-architecture.7.xml @@ -441,17 +441,28 @@ A localnet logical switch port bridges a logical switch to a -physical VLAN. A logical switch with a localnet LSP should -have only one other LSP. Some kinds of gateways (see Gateways -below) use a logical switch with a router port as the second LSP. On the -other hand, when the second LSP is a VIF, the logical switch is not really -a logical network, since it is bridged to the physical network rather than -insulated from it, and therefore cannot have independent but overlapping IP -address namespaces, etc. (A deployment might nevertheless choose such a -configuration to take advantage of the OVN control plane and features such -as port security and ACLs.) +physical VLAN. Any given logical switch should have no more than one +localnet port. Such a logical switch is used in two +scenarios: + + + With one or more router logical switch ports, to attach L3 + gateway routers and distributed gateways to a physical network. + + + + With one or more VIF logical switch ports, to attach VMs or containers + directly to a physical network. In this case, the logical switch is not + really logical, since it is bridged to the physical network rather than + insulated from it, and therefore cannot have independent but overlapping + IP address namespaces, etc. A deployment might nevertheless choose such + a configuration to take advantage of the OVN control plane and features + such as port security and ACLs. + + + A localport logical switch port is a special kind of VIF logical switch port. These ports are present in every chassis, not bound -- 2.24.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v2 1/7] xml2nroff: Properly support .
uses .SU but xml2nroff didn't define .SU. Signed-off-by: Ben Pfaff --- build-aux/xml2nroff | 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/build-aux/xml2nroff b/build-aux/xml2nroff index bd4e87928e4f..764ace9b19ba 100755 --- a/build-aux/xml2nroff +++ b/build-aux/xml2nroff @@ -90,8 +90,14 @@ def manpage_to_nroff(xml_file, subst, include_path, version=None): . I "\\$1" . RE .. -''' % (build.nroff.text_to_nroff(program), build.nroff.text_to_nroff(section), - build.nroff.text_to_nroff(title), build.nroff.text_to_nroff(version)) +.de SU +. PP +. I "$1" +.. +build/''' % (build.nroff.text_to_nroff(program), + build.nroff.text_to_nroff(section), + build.nroff.text_to_nroff(title), + build.nroff.text_to_nroff(version)) s += build.nroff.block_xml_to_nroff(doc.childNodes) + "\n" -- 2.24.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v2 2/7] ovn-architecture: Flip network diagrams.
Most network diagrams show end-hosts at the bottom ("south").
Signed-off-by: Ben Pfaff
---
ovn-architecture.7.xml | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/ovn-architecture.7.xml b/ovn-architecture.7.xml
index 415c895e497f..d32978a9d5be 100644
--- a/ovn-architecture.7.xml
+++ b/ovn-architecture.7.xml
@@ -598,17 +598,17 @@
- LS1 ... LSn
- |||
- +++
+LSlocal
|
- LR1
+ GLR
|
LSjoin
|
- GLR
+ LR1
|
-LSlocal
+ +++
+ |||
+ LS1 ... LSn
@@ -650,13 +650,13 @@
- LS1 ... LSn
- |||
- +++
+LSlocal
|
LR1
|
-LSlocal
+ +++
+ |||
+ LS1 ... LSn
--
2.24.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/5] ovn-nb: Consolidate documentation of distributed gateway ports.
On Mon, Mar 16, 2020 at 02:57:25PM +0530, Numan Siddique wrote: > On Sat, Mar 14, 2020 at 4:03 AM Ben Pfaff wrote: > > > > The documentation of distributed gateway ports was in two places: one > > place for the main documentation, another for the options. This puts > > it all in one place to make it easier to follow. > > > > Signed-off-by: Ben Pfaff ... > The option - "reside-on-redirect-chassis" applies to normal logical router > ports and not to a distributed gateway port. > > This option is considered by ovn-northd only if the >- router port's logical switch has a localnet port >- The router to which this port is connected has a distributed > gateway port in it. > > (The reply here some more details - > https://mail.openvswitch.org/pipermail/ovs-dev/2020-March/368595.html) OK, I have spent a lot of time reading code and documentation and commit message nd now I think I understand, mostly. I rewrote everything entirely for v2, which I will send soon. Please do review it. I'm not going to push it without your feedback. The following may be more of a question for Ankur. I don't fully understand options:redirect-type=bridged. As I understand it, the problem that this (and reside-on-redirect-chassis) is intended to solve is an MTU problem. I see how reside-on-redirect-chassis solves the MTU problem, by avoiding tunneling entirely. For redirect-type, it seems like the MTU problem would still be there for east-west communication, since that still goes through tunnels. Reducing the VM's MTU would solve that problem, but it seems like it would also solve the problem that redirect-type/reside-on-redirect-chassis solves. Thanks, Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] hmap.h: Fix incorrect expression
>From Ben Pfaff b...@ovn.org Coverity reports incorrect expression for HMAP_FOR_EACH_WITH_HASH macro This patch fixes this issue "make check" passes for this change Coverity reports 80 errors resolved Signed-off-by: Usman Ansari --- include/openvswitch/hmap.h 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/openvswitch/hmap.h b/include/openvswitch/hmap.h index 8aea9c2..f48359f 100644 --- a/include/openvswitch/hmap.h +++ b/include/openvswitch/hmap.h @@ -136,7 +136,7 @@ struct hmap_node *hmap_random_node(const struct hmap *); */ #define HMAP_FOR_EACH_WITH_HASH(NODE, MEMBER, HASH, HMAP) \ for (INIT_CONTAINER(NODE, hmap_first_with_hash(HMAP, HASH), MEMBER); \ - (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || (NODE = NULL); \ + (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER)) || ((NODE = NULL), false); \ ASSIGN_CONTAINER(NODE, hmap_next_with_hash(&(NODE)->MEMBER), \ MEMBER)) #define HMAP_FOR_EACH_IN_BUCKET(NODE, MEMBER, HASH, HMAP) \ --- ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 5/5] docs: Acknowledge in more places that localnet isn't the only option.
On Fri, Mar 13, 2020 at 05:13:02PM -0700, Han Zhou wrote: > On Fri, Mar 13, 2020 at 3:33 PM Ben Pfaff wrote: > > > > Signed-off-by: Ben Pfaff > > --- > > ovn-architecture.7.xml | 14 +++--- > > ovn-nb.xml | 6 +++--- > > 2 files changed, 10 insertions(+), 10 deletions(-) > > > > diff --git a/ovn-architecture.7.xml b/ovn-architecture.7.xml > > index 450b40ab7a8b..810937ea92cb 100644 > > --- a/ovn-architecture.7.xml > > +++ b/ovn-architecture.7.xml > > @@ -1499,10 +1499,11 @@ > > the VM or container to the outside world should be processed > > completely on that VM's or container's hypervisor, eventually > > traversing a localnet port instance on that hypervisor to the > > -physical network. Whenever possible, packets from the outside > > -world to a VM or container should be directed through the physical > > -network directly to the VM's or container's hypervisor, where the > > -packet will enter the integration bridge through a localnet port. > > +physical network or a connection to a different OVN deployment. > > +Whenever possible, packets from the outside world to a VM or > > +container should be directed through the physical network directly > > +to the VM's or container's hypervisor, where the packet will enter > > +the integration bridge through a localnet port. > > > > > For interconnection, tunnels are used between GWs instead of localnet > ports, so "traversing a localnet port" and "through a localnet port" > described above are still not accurate. Thanks. This is what I put into v2: The primary design goal of distributed gateway ports is to allow as much traffic as possible to be handled locally on the hypervisor where a VM or container resides. Whenever possible, packets from the VM or container to the outside world should be processed completely on that VM's or container's hypervisor, eventually traversing a localnet port instance or a tunnel to the physical network or a different OVN deployment. Whenever possible, packets from the outside world to a VM or container should be directed through the physical network directly to the VM's or container's hypervisor. > > > > @@ -1788,9 +1789,8 @@ > > > > > > > > -If the logical router doesn't have a distributed gateway port > connecting > > -to the localnet logical switch which provides external connectivity, > > -then this option is ignored by OVN. > > +OVN ignores this option if the logical router doesn't have a > distributed > > +gateway port that provides external connectivity. > > > > I think we shouldn't change the text here. The "reside-on-redirect-chassis" > option is set to a logical router port instead of a router, and it does > require a localnet-connected switch connected to this logical router port. > The use case is for "Centralized routing for localnet VLAN tagged logical > switches connected to a logical router", so it makes sense to require > localnet connection. OK. I dropped this change. I'll send v2 soon. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 ovn] Documentation: Change 'Open vSwitch' for 'OVN and logo
On Wed, Mar 18, 2020 at 05:31:23PM +0100, Daniel Alvarez Sanchez wrote: > On Tue, Mar 17, 2020 at 6:57 PM Ben Pfaff wrote: > > > On Tue, Mar 17, 2020 at 06:22:07PM +0100, Daniel Alvarez wrote: > > > The current version of the documentation is still using the > > > Open vSwitch logo and includes some references to OVS that > > > should be changed by OVN. > > > > > > Signed-off-by: Daniel Alvarez > > > > Here's the original SVG for the logo. > > > > Thanks Ben, I picked the OVN log and resized to 200x200 as per the Sphinx > doc [0]. > I'm a bit puzzled as the OVS logo [1] is 502x334 but somehow it works well > in the OVS website. > > Do you have any suggestions? Thanks a lot! I'm very pleased to see that you and others are working on this! I don't have suggestions right now. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] Consulta
Consulta ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 ovn] Documentation: Change 'Open vSwitch' for 'OVN and logo
On Tue, Mar 17, 2020 at 6:57 PM Ben Pfaff wrote: > On Tue, Mar 17, 2020 at 06:22:07PM +0100, Daniel Alvarez wrote: > > The current version of the documentation is still using the > > Open vSwitch logo and includes some references to OVS that > > should be changed by OVN. > > > > Signed-off-by: Daniel Alvarez > > Here's the original SVG for the logo. > Thanks Ben, I picked the OVN log and resized to 200x200 as per the Sphinx doc [0]. I'm a bit puzzled as the OVS logo [1] is 502x334 but somehow it works well in the OVS website. Do you have any suggestions? Thanks a lot! [0] https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-html-logo [1] https://github.com/openvswitch/ovs/blob/master/Documentation/_static/logo.png ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netdev: Fix integer handling issue.
On Wed, Mar 18, 2020 at 04:53:27PM +0100, Ilya Maximets wrote:
> On 3/18/20 4:36 PM, Ben Pfaff wrote:
> > On Wed, Mar 18, 2020 at 08:34:16AM -0700, William Tu wrote:
> >> On Wed, Mar 18, 2020 at 7:59 AM Ilya Maximets wrote:
> >>>
> >>> On 3/18/20 12:31 AM, William Tu wrote:
> Coverity CID 279497 reports "Operands don't affect result".
> Because flow->ct_state is uint8_t and DP_NETDEV_CS_UNSUPPORTED_MASK
> is '0xff00'. So remove the statement.
>
> Cc: Usman Ansari
> Signed-off-by: William Tu
> ---
> lib/dpif-netdev.c | 4
> 1 file changed, 4 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index a798db45d9cb..0e2678d002d5 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -3224,10 +3224,6 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr
> *key, uint32_t key_len,
> return EINVAL;
> }
>
> -if (flow->ct_state & DP_NETDEV_CS_UNSUPPORTED_MASK) {
> -return EINVAL;
> -}
> -
> >>>
> >>> I'm not sure if we need to remove this. This code doesn't make any harm
> >>> and most likely compiled out. I agree that it doesn't change any logic
> >>> in this function, but in case someone will try to add new flags or change
> >>> the type of ct_state we will be safe and will reject all the unknown
> >>> flags.
> >>> Without this code we'll have to catch this case somehow on code review and
> >>> re-introduce this check or implement missing functionality.
> >>>
> >>> One more thing is that DP_NETDEV_CS_UNSUPPORTED_MASK definition becomes
> >>> unused and should be removed along with _SUPPORTED_MASK.
> >>
> >> Good point.
> >>
> >>>
> >>> So, I'd rather not touch this and just mark this code as OK for coverity
> >>> scanner. But if you want to remove, please, clean up other parts and
> >>> add a build assert for the ct_state size and flags, so any disruptive
> >>> change
> >>> will be caught by the developer of this change.
> >>>
> >> OK thanks!
> >> Let's keep this code block as it is now.
> >
> > I was surprised to hear that it doesn't have any effect. Adding a
> > comment might be helpful.
> >
>
> DP_NETDEV_CS_UNSUPPORTED_MASK was introduced at the time when dpif-netdev
> didn't
> support NAT. After the NAT support implementation in commit
> 4cddb1f0d837 ("dpdk: Parse NAT netlink for userspace datapath.") this mask is
> just
> a zero in the lowest 8 bits, i.e. all current flags are supported.
>
> I'm not sure why it's casted to uint32_t, though, or why flow->ct_state is 8
> bits
> only. packets.h has similar mask and it's casted to uint32_t too. The main
> concern
> here is that it seems like ct_state is 32bit in netlink. That produces
> misunderstanding
> and makes me nervous about potential issues.
>
> flow->ct_state is 8 bits long and mask is zero there, so this 'if' statement
> is
> always false.
Oh, I understand the reason, but from glancing at the code it's not
obvious.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netdev: Fix integer handling issue.
On 3/18/20 4:36 PM, Ben Pfaff wrote:
> On Wed, Mar 18, 2020 at 08:34:16AM -0700, William Tu wrote:
>> On Wed, Mar 18, 2020 at 7:59 AM Ilya Maximets wrote:
>>>
>>> On 3/18/20 12:31 AM, William Tu wrote:
Coverity CID 279497 reports "Operands don't affect result".
Because flow->ct_state is uint8_t and DP_NETDEV_CS_UNSUPPORTED_MASK
is '0xff00'. So remove the statement.
Cc: Usman Ansari
Signed-off-by: William Tu
---
lib/dpif-netdev.c | 4
1 file changed, 4 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index a798db45d9cb..0e2678d002d5 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -3224,10 +3224,6 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr
*key, uint32_t key_len,
return EINVAL;
}
-if (flow->ct_state & DP_NETDEV_CS_UNSUPPORTED_MASK) {
-return EINVAL;
-}
-
>>>
>>> I'm not sure if we need to remove this. This code doesn't make any harm
>>> and most likely compiled out. I agree that it doesn't change any logic
>>> in this function, but in case someone will try to add new flags or change
>>> the type of ct_state we will be safe and will reject all the unknown flags.
>>> Without this code we'll have to catch this case somehow on code review and
>>> re-introduce this check or implement missing functionality.
>>>
>>> One more thing is that DP_NETDEV_CS_UNSUPPORTED_MASK definition becomes
>>> unused and should be removed along with _SUPPORTED_MASK.
>>
>> Good point.
>>
>>>
>>> So, I'd rather not touch this and just mark this code as OK for coverity
>>> scanner. But if you want to remove, please, clean up other parts and
>>> add a build assert for the ct_state size and flags, so any disruptive change
>>> will be caught by the developer of this change.
>>>
>> OK thanks!
>> Let's keep this code block as it is now.
>
> I was surprised to hear that it doesn't have any effect. Adding a
> comment might be helpful.
>
DP_NETDEV_CS_UNSUPPORTED_MASK was introduced at the time when dpif-netdev didn't
support NAT. After the NAT support implementation in commit
4cddb1f0d837 ("dpdk: Parse NAT netlink for userspace datapath.") this mask is
just
a zero in the lowest 8 bits, i.e. all current flags are supported.
I'm not sure why it's casted to uint32_t, though, or why flow->ct_state is 8
bits
only. packets.h has similar mask and it's casted to uint32_t too. The main
concern
here is that it seems like ct_state is 32bit in netlink. That produces
misunderstanding
and makes me nervous about potential issues.
flow->ct_state is 8 bits long and mask is zero there, so this 'if' statement is
always false.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5] db-ctl-base: add uuid specified method for create cmd
I guess it's a bug. Have you posted the code you tested? Please post a v6 that includes the test that fails. On Wed, Mar 18, 2020 at 11:03:45PM +0800, Timo_Liu wrote: > Hi Ben: When using both --id and --row_uuid, I have found the specified uuid only worked for table whose "is_root" flag is true. For non root table, such as mirror table, when we execute table create with --id and --row_uuid，the json transaction includes two ops: insert mirror table (with uuid specified by --row_uuid)， and update Bridge table column, But for the update operation, the row->uuid is not the specified uuid any more. I don't know why the "uuid-name" key for "insert" operation can tranmit its uuid value to row->new_datum for "update" operatoin. Can you give me some advice ? Thanks Timo Re: Re: [ovs-dev] [PATCH v5] db-ctl-base: add uuid specified method for create cmdOn Mon, Mar 16, 2020 at 09:31:48AM +0800, Timo_Liu wrote: > One point that we should confirm with you: if --id && > --row-uuid both specified, we should use UUID from --row-uuid to > create table, am I right ? Yes. > Also, we should add test in which ovsdb unit test file ?(ovs-vsctl.at or > ovsdb-execution.at) I think that ovs-vsctl.at is a good place for testing ovs-vsctl features. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netdev: Fix integer handling issue.
On Wed, Mar 18, 2020 at 08:34:16AM -0700, William Tu wrote:
> On Wed, Mar 18, 2020 at 7:59 AM Ilya Maximets wrote:
> >
> > On 3/18/20 12:31 AM, William Tu wrote:
> > > Coverity CID 279497 reports "Operands don't affect result".
> > > Because flow->ct_state is uint8_t and DP_NETDEV_CS_UNSUPPORTED_MASK
> > > is '0xff00'. So remove the statement.
> > >
> > > Cc: Usman Ansari
> > > Signed-off-by: William Tu
> > > ---
> > > lib/dpif-netdev.c | 4
> > > 1 file changed, 4 deletions(-)
> > >
> > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > > index a798db45d9cb..0e2678d002d5 100644
> > > --- a/lib/dpif-netdev.c
> > > +++ b/lib/dpif-netdev.c
> > > @@ -3224,10 +3224,6 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr
> > > *key, uint32_t key_len,
> > > return EINVAL;
> > > }
> > >
> > > -if (flow->ct_state & DP_NETDEV_CS_UNSUPPORTED_MASK) {
> > > -return EINVAL;
> > > -}
> > > -
> >
> > I'm not sure if we need to remove this. This code doesn't make any harm
> > and most likely compiled out. I agree that it doesn't change any logic
> > in this function, but in case someone will try to add new flags or change
> > the type of ct_state we will be safe and will reject all the unknown flags.
> > Without this code we'll have to catch this case somehow on code review and
> > re-introduce this check or implement missing functionality.
> >
> > One more thing is that DP_NETDEV_CS_UNSUPPORTED_MASK definition becomes
> > unused and should be removed along with _SUPPORTED_MASK.
>
> Good point.
>
> >
> > So, I'd rather not touch this and just mark this code as OK for coverity
> > scanner. But if you want to remove, please, clean up other parts and
> > add a build assert for the ct_state size and flags, so any disruptive change
> > will be caught by the developer of this change.
> >
> OK thanks!
> Let's keep this code block as it is now.
I was surprised to hear that it doesn't have any effect. Adding a
comment might be helpful.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netdev: Fix integer handling issue.
On Wed, Mar 18, 2020 at 7:59 AM Ilya Maximets wrote:
>
> On 3/18/20 12:31 AM, William Tu wrote:
> > Coverity CID 279497 reports "Operands don't affect result".
> > Because flow->ct_state is uint8_t and DP_NETDEV_CS_UNSUPPORTED_MASK
> > is '0xff00'. So remove the statement.
> >
> > Cc: Usman Ansari
> > Signed-off-by: William Tu
> > ---
> > lib/dpif-netdev.c | 4
> > 1 file changed, 4 deletions(-)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index a798db45d9cb..0e2678d002d5 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -3224,10 +3224,6 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr
> > *key, uint32_t key_len,
> > return EINVAL;
> > }
> >
> > -if (flow->ct_state & DP_NETDEV_CS_UNSUPPORTED_MASK) {
> > -return EINVAL;
> > -}
> > -
>
> I'm not sure if we need to remove this. This code doesn't make any harm
> and most likely compiled out. I agree that it doesn't change any logic
> in this function, but in case someone will try to add new flags or change
> the type of ct_state we will be safe and will reject all the unknown flags.
> Without this code we'll have to catch this case somehow on code review and
> re-introduce this check or implement missing functionality.
>
> One more thing is that DP_NETDEV_CS_UNSUPPORTED_MASK definition becomes
> unused and should be removed along with _SUPPORTED_MASK.
Good point.
>
> So, I'd rather not touch this and just mark this code as OK for coverity
> scanner. But if you want to remove, please, clean up other parts and
> add a build assert for the ct_state size and flags, so any disruptive change
> will be caught by the developer of this change.
>
OK thanks!
Let's keep this code block as it is now.
William
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] conntrack: Reset ct_state when entering a new zone.
On Wed, Mar 18, 2020 at 02:57:53PM +0100, Ilya Maximets wrote: > On 3/5/20 12:28 PM, Dumitru Ceara wrote: > > When a new conntrack zone is entered, the ct_state field is zeroed in > > order to avoid using state information from different zones. ... > Regarding the issue. I've spent some time exploring the conntrack code > and also researching the original patch that introduced this code. > The issue was raised during the review of the original patch 286de2729955 > by Daniele: https://patchwork.ozlabs.org/patch/743108/ > And Darrell said that he actually had the similar code that clears ct_state > during zone transition at the beginning of process_one(). But, he decided > that most of such issues are likely configuration bugs that should by raised > to user with warnings. > However, such warnings was never introduced and since this causes a real > issue in OVN we should actually have this clearing of conntrack state on > zone transitioning. > > Acked-by: Ilya Maximets > > Darrell, Ben, I'd like to have some comments on this from you since I'm > not an expert in this area. Otherwise, I'm going to apply this patch on > next week. I *think* that this new behavior matches that of the kernel datapath. In __ovs_ct_lookup(), I believe that a change in zone would cause skb_nfct_cached() to return false, causing the code to reset ct_state to 0. If so, then it makes sense for the userspace datapath to behave similarly. Darrell? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] conntrack: Fix NULL pointer dereference.
On Wed, Mar 18, 2020 at 01:49:48PM +0100, Ilya Maximets wrote:
> On 3/18/20 12:12 AM, William Tu wrote:
> > Coverity CID 279957 reports NULL pointer derefence when
> > 'conn' is NULL and calling ct_print_conn_info.
> >
> > Cc: Usman Ansari
> > Signed-off-by: William Tu
> > ---
> > lib/conntrack.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > index ff5a89457c0a..001a37ff6aff 100644
> > --- a/lib/conntrack.c
> > +++ b/lib/conntrack.c
> > @@ -1302,7 +1302,7 @@ process_one(struct conntrack *ct, struct dp_packet
> > *pkt,
> > if (!conn) {
> > pkt->md.ct_state |= CS_TRACKED | CS_INVALID;
> > char *log_msg = xasprintf("Missing master conn %p",
> > rev_conn);
> > -ct_print_conn_info(conn, log_msg, VLL_INFO, true, true);
> > +ct_print_conn_info(rev_conn, log_msg, VLL_INFO, true,
> > true);
> > free(log_msg);
> > return;
> > }
> >
>
> Hi.
>
> This issue is addressed as part of the following patch:
> https://patchwork.ozlabs.org/patch/1249513/
> I'm not sure if we need to split it and fix this issue separately.
> Thoughts?
It seems like a separate issue to me, just located in nearby code.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] bugtool: Fix for Python3
Currently ovs-bugtool tool doesn't start on Python3.
This commit fixes ovs-bugtool to make it works on Python.3
Reported-at: https://bugzilla.redhat.com/1809241
Reported-by: Flavio Leitner
Signed-off-by: Timothy Redaelli
---
utilities/bugtool/ovs-bugtool.in | 27 +--
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/utilities/bugtool/ovs-bugtool.in b/utilities/bugtool/ovs-bugtool.in
index e55bfc2ed..7021305c4 100755
--- a/utilities/bugtool/ovs-bugtool.in
+++ b/utilities/bugtool/ovs-bugtool.in
@@ -33,8 +33,7 @@
# or func_output().
#
-import StringIO
-import commands
+from io import StringIO
import fcntl
import getopt
import hashlib
@@ -48,7 +47,7 @@ import warnings
import zipfile
from select import select
from signal import SIGTERM
-from subprocess import PIPE, Popen
+from subprocess import PIPE, Popen, check_output
from xml.dom.minidom import getDOMImplementation, parse
@@ -704,7 +703,7 @@ exclude those logs from the archive.
# permit the user to filter out data
# We cannot use iteritems, since we modify 'data' as we pass through
-for (k, v) in sorted(data.items()):
+for (k, v) in data.items():
cap = v['cap']
if 'filename' in v:
key = k[0]
@@ -782,7 +781,7 @@ def dump_scsi_hosts(cap):
def module_info(cap):
-output = StringIO.StringIO()
+output = StringIO()
modules = open(PROC_MODULES, 'r')
procs = []
@@ -806,7 +805,7 @@ def multipathd_topology(cap):
def dp_list():
-output = StringIO.StringIO()
+output = StringIO()
procs = [ProcOutput([OVS_DPCTL, 'dump-dps'],
caps[CAP_NETWORK_STATUS][MAX_TIME], output)]
@@ -828,7 +827,7 @@ def collect_ovsdb():
if os.path.isfile(OPENVSWITCH_COMPACT_DB):
os.unlink(OPENVSWITCH_COMPACT_DB)
-output = StringIO.StringIO()
+output = StringIO()
max_time = 5
procs = [ProcOutput(['ovsdb-tool', 'compact',
OPENVSWITCH_CONF_DB, OPENVSWITCH_COMPACT_DB],
@@ -871,7 +870,7 @@ def fd_usage(cap):
def dump_rdac_groups(cap):
-output = StringIO.StringIO()
+output = StringIO()
procs = [ProcOutput([MPPUTIL, '-a'], caps[cap][MAX_TIME], output)]
run_procs([procs])
@@ -1095,7 +1094,7 @@ def make_inventory(inventory, subdir):
s.setAttribute('date', time.strftime('%c'))
s.setAttribute('hostname', platform.node())
s.setAttribute('uname', ' '.join(platform.uname()))
-s.setAttribute('uptime', commands.getoutput(UPTIME))
+s.setAttribute('uptime', check_output(UPTIME).decode())
document.getElementsByTagName(INVENTORY_XML_ROOT)[0].appendChild(s)
map(lambda k_v: inventory_entry(document, subdir, k_v[0], k_v[1]),
@@ -1301,7 +1300,7 @@ class ProcOutput(object):
line = self.proc.stdout.readline()
else:
line = self.proc.stdout.read(self.bufsize)
-if line == '':
+if line == b'':
# process exited
self.proc.stdout.close()
self.status = self.proc.wait()
@@ -1311,7 +1310,7 @@ class ProcOutput(object):
if self.filter:
line = self.filter(line, self.filter_state)
if self.inst:
-self.inst.write(line)
+self.inst.write(line.decode())
def run_procs(procs):
@@ -1391,13 +1390,13 @@ def get_free_disk_space(path):
return s.f_frsize * s.f_bfree
-class StringIOmtime(StringIO.StringIO):
+class StringIOmtime(StringIO):
def __init__(self, buf=''):
-StringIO.StringIO.__init__(self, buf)
+StringIO.__init__(self, buf)
self.mtime = time.time()
def write(self, s):
-StringIO.StringIO.write(self, s)
+StringIO.write(self, s)
self.mtime = time.time()
--
2.25.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5] db-ctl-base: add uuid specified method for create cmd
Hi Ben: When using both --id and --row_uuid, I have found the specified uuid only worked for table whose "is_root" flag is true. For non root table, such as mirror table, when we execute table create with --id and --row_uuid，the json transaction includes two ops: insert mirror table (with uuid specified by --row_uuid)， and update Bridge table column, But for the update operation, the row->uuid is not the specified uuid any more. I don't know why the "uuid-name" key for "insert" operation can tranmit its uuid value to row->new_datum for "update" operatoin. Can you give me some advice ? Thanks Timo Re: Re: [ovs-dev] [PATCH v5] db-ctl-base: add uuid specified method for create cmdOn Mon, Mar 16, 2020 at 09:31:48AM +0800, Timo_Liu wrote: > One point that we should confirm with you: if --id && > --row-uuid both specified, we should use UUID from --row-uuid to > create table, am I right ? Yes. > Also, we should add test in which ovsdb unit test file ?(ovs-vsctl.at or > ovsdb-execution.at) I think that ovs-vsctl.at is a good place for testing ovs-vsctl features. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netdev: Fix integer handling issue.
On 3/18/20 12:31 AM, William Tu wrote:
> Coverity CID 279497 reports "Operands don't affect result".
> Because flow->ct_state is uint8_t and DP_NETDEV_CS_UNSUPPORTED_MASK
> is '0xff00'. So remove the statement.
>
> Cc: Usman Ansari
> Signed-off-by: William Tu
> ---
> lib/dpif-netdev.c | 4
> 1 file changed, 4 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index a798db45d9cb..0e2678d002d5 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -3224,10 +3224,6 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr
> *key, uint32_t key_len,
> return EINVAL;
> }
>
> -if (flow->ct_state & DP_NETDEV_CS_UNSUPPORTED_MASK) {
> -return EINVAL;
> -}
> -
I'm not sure if we need to remove this. This code doesn't make any harm
and most likely compiled out. I agree that it doesn't change any logic
in this function, but in case someone will try to add new flags or change
the type of ct_state we will be safe and will reject all the unknown flags.
Without this code we'll have to catch this case somehow on code review and
re-introduce this check or implement missing functionality.
One more thing is that DP_NETDEV_CS_UNSUPPORTED_MASK definition becomes
unused and should be removed along with _SUPPORTED_MASK.
So, I'd rather not touch this and just mark this code as OK for coverity
scanner. But if you want to remove, please, clean up other parts and
add a build assert for the ct_state size and flags, so any disruptive change
will be caught by the developer of this change.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ofp-actions: Fix memory leak.
On 3/17/2020 4:31 PM, William Tu wrote:
Coverity CID 279274 reports leaking previously allocated
'error' buffer when 'return xasprintf("input too big");'.
Cc: Usman Ansari
Signed-off-by: William Tu
---
lib/ofp-actions.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index ddef3b0c8780..ef8b2b4527f9 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -6657,6 +6657,7 @@ parse_CT(char *arg, const struct ofpact_parse_params *pp)
}
if (ofpbuf_oversized(pp->ofpacts)) {
+free(error);
return xasprintf("input too big");
}
Reviewed-by: Greg Rose
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netdev: Fix integer handling issue.
On 3/17/2020 4:31 PM, William Tu wrote:
Coverity CID 279497 reports "Operands don't affect result".
Because flow->ct_state is uint8_t and DP_NETDEV_CS_UNSUPPORTED_MASK
is '0xff00'. So remove the statement.
Cc: Usman Ansari
Signed-off-by: William Tu
---
lib/dpif-netdev.c | 4
1 file changed, 4 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index a798db45d9cb..0e2678d002d5 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -3224,10 +3224,6 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr *key,
uint32_t key_len,
return EINVAL;
}
-if (flow->ct_state & DP_NETDEV_CS_UNSUPPORTED_MASK) {
-return EINVAL;
-}
-
return 0;
}
Reviewed-by: Greg Rose
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v7 ovn 2/2] northd: add logical flows for dhcpv6 pfd parsing
Introduce logical flows in ovn router pipeline in order to parse dhcpv6
advertise/reply from IPv6 prefix delegation router.
Do not overwrite ipv6_ra_pd_list info in options column of SB port_binding
table written by ovn-controller
Introduce ipv6_prefix column in NB Logical_router_port table to report
IPv6 prefix received from delegation router to the CMS
Signed-off-by: Lorenzo Bianconi
---
northd/ovn-northd.8.xml | 8 +++
northd/ovn-northd.c | 95 --
ovn-nb.ovsschema| 7 ++-
ovn-nb.xml | 21 +++
tests/atlocal.in| 5 +-
tests/system-ovn.at | 127
6 files changed, 255 insertions(+), 8 deletions(-)
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 9b44720d1..a674798a5 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -1658,6 +1658,14 @@ next;
+
+ A priority-100 flow parses DHCPv6 replies from IPv6 prefix
+ delegation routers (udp.src == 547 &&
+ udp.dst == 546). The handle_dhcpv6_reply
+ is used to send IPv6 prefix delegation messages to the delegation
+ router.
+
+
A priority-87 flow explicitly allows IPv6 multicast traffic that is
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 4f94680b5..96101bc29 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -2689,6 +2689,33 @@ op_get_name(const struct ovn_port *op)
return name;
}
+static void
+ovn_update_ipv6_prefix(struct hmap *ports)
+{
+const struct ovn_port *op;
+HMAP_FOR_EACH (op, key_node, ports) {
+if (!op->nbrp) {
+continue;
+}
+
+char prefix[IPV6_SCAN_LEN + 6];
+unsigned aid;
+const char *ipv6_pd_list = smap_get(&op->sb->options,
+"ipv6_ra_pd_list");
+if (!ipv6_pd_list ||
+!ovs_scan(ipv6_pd_list, "%u:%s", &aid, prefix)) {
+continue;
+}
+
+struct sset ipv6_prefix_set = SSET_INITIALIZER(&ipv6_prefix_set);
+sset_add(&ipv6_prefix_set, prefix);
+nbrec_logical_router_port_set_ipv6_prefix(op->nbrp,
+sset_array(&ipv6_prefix_set),
+sset_count(&ipv6_prefix_set));
+sset_destroy(&ipv6_prefix_set);
+}
+}
+
static void
ovn_port_update_sbrec(struct northd_context *ctx,
struct ovsdb_idl_index *sbrec_chassis_by_name,
@@ -2819,6 +2846,13 @@ ovn_port_update_sbrec(struct northd_context *ctx,
smap_add(&new, "l3gateway-chassis", chassis_name);
}
}
+
+const char *ipv6_pd_list = smap_get(&op->sb->options,
+"ipv6_ra_pd_list");
+if (ipv6_pd_list) {
+smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list);
+}
+
sbrec_port_binding_set_options(op->sb, &new);
smap_destroy(&new);
@@ -4673,12 +4707,12 @@ build_pre_acls(struct ovn_datapath *od, struct hmap
*lflows)
* unreachable packets. */
ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 110,
"nd || nd_rs || nd_ra || icmp4.type == 3 || "
- "icmp6.type == 1 || (tcp && tcp.flags == 20)",
- "next;");
+ "icmp6.type == 1 || (tcp && tcp.flags == 20) || "
+ "(udp && udp.src == 546 && udp.dst == 547)", "next;");
ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_ACL, 110,
"nd || nd_rs || nd_ra || icmp4.type == 3 || "
- "icmp6.type == 1 || (tcp && tcp.flags == 20)",
- "next;");
+ "icmp6.type == 1 || (tcp && tcp.flags == 20) ||"
+ "(udp && udp.src == 546 && udp.dst == 547)", "next;");
/* Ingress and Egress Pre-ACL Table (Priority 100).
*
@@ -7702,6 +7736,11 @@ copy_ra_to_sb(struct ovn_port *op, const char
*address_mode)
}
ds_put_format(&s, "%s/%u ", addrs->network_s, addrs->plen);
}
+
+const char *ra_pd_list = smap_get(&op->sb->options, "ipv6_ra_pd_list");
+if (ra_pd_list) {
+ds_put_cstr(&s, ra_pd_list);
+}
/* Remove trailing space */
ds_chomp(&s, ' ');
smap_add(&options, "ipv6_ra_prefixes", ds_cstr(&s));
@@ -8446,7 +8485,34 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap
*ports,
free(snat_ips);
}
-/* Logical router ingress table 3: IP Input for IPv6. */
+/* DHCPv6 reply handling */
+HMAP_FOR_EACH (op, key_node, ports) {
+if (!op->nbrp) {
+continue;
+}
+
+if (op->derived) {
+continue;
+}
+
+struct lport_addresses lrp_networks;
+if (!extract_lrp_networks(op->nbrp, &lrp_networks)) {
+continue;
+}
+
+
[ovs-dev] [PATCH v7 ovn 0/2] Add IPv6 Prefix delegation (RFC3633)
Introduce IPv6 Prefix delegation state machine according to RFC 3633 https://tools.ietf.org/html/rfc3633. Add handle_dhcpv6_reply controller action to parse advertise/reply from IPv6 delegation server. Introduce logical flows in ovn router pipeline in order to parse dhcpv6 advertise/reply from IPv6 prefix delegation router. This series relies on the following OVS commit: https://github.com/openvswitch/ovs/commit/cec89046f72cb044b068ba6a4e30dbcc4292c4c1 Changes since v6: - fix documentation - convert handle_dhcpv6_reply from netsted to plain action - code reworking to reduce cpu utilization - insert configured prefix as hint in dhcp pd messages Changes since v5: - introduce ipv6_prefix column in logical_router_port table to save ipv6 prefix received from delegation router Changes since v4: - improve unit test support - fix ovn-controller crash - confifure prefixes received from delegation router in RA - allow the requesting router to rely on lla address for PD protocol Changes since v3: - cosmetics - add a provider bridge in the unit-test deployment and add a localnet port to the deployment to access the underlay network - request IPv6 prefix even for bar router logical port in the unit-test deployment Changes since v2: - add unitest support in system-ovn.at Changes since v1: - rebase on top of ovn master branch - request an IPv6 prefix for each 'downstream' logical router port marked with prefix set to true - add missing documentation - rename dhcp6_server_pkt in handle_dhcpv6_reply Lorenzo Bianconi (2): controller: add ipv6 prefix delegation state machine northd: add logical flows for dhcpv6 pfd parsing controller/pinctrl.c| 659 +++- controller/pinctrl.h| 1 + include/ovn/actions.h | 8 +- lib/actions.c | 16 + lib/ovn-l7.h| 19 ++ northd/ovn-northd.8.xml | 8 + northd/ovn-northd.c | 95 +- ovn-nb.ovsschema| 7 +- ovn-nb.xml | 21 ++ ovn-sb.xml | 18 ++ tests/atlocal.in| 5 +- tests/ovn.at| 4 + tests/system-ovn.at | 127 utilities/ovn-trace.c | 2 + 14 files changed, 974 insertions(+), 16 deletions(-) -- 2.25.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v7 ovn 1/2] controller: add ipv6 prefix delegation state machine
Introduce IPv6 Prefix delegation state machine according to RFC 3633
https://tools.ietf.org/html/rfc3633.
Add handle_dhcpv6_reply controller action to parse advertise/reply from
IPv6 delegation server. Advertise/reply are parsed running respectively:
- pinctrl_parse_dhcv6_advt
- pinctrl_parse_dhcv6_reply
The IPv6 requesting router starts sending dhcpv6 solicit through the logical
router port marked with ipv6_prefix_delegation set to true.
An IPv6 prefix will be requested for each logical router port marked
with "prefix" set to true in option column of logical router port table.
Save IPv6 prefix received by IPv6 delegation router in the options columns of
SB port binding table in order to be reused by Router Advertisement framework
run by ovn logical router pipeline.
IPv6 Prefix delegation state machine is enabled on Gateway Router or on
a Gateway Router Port
Signed-off-by: Lorenzo Bianconi
---
controller/pinctrl.c | 659 +-
controller/pinctrl.h | 1 +
include/ovn/actions.h | 8 +-
lib/actions.c | 16 +
lib/ovn-l7.h | 19 ++
ovn-sb.xml| 18 ++
tests/ovn.at | 4 +
utilities/ovn-trace.c | 2 +
8 files changed, 719 insertions(+), 8 deletions(-)
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 8bf19776c..c705a3244 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -270,6 +270,19 @@ static void pinctrl_ip_mcast_handle(
const struct match *md,
struct ofpbuf *userdata);
+static void init_ipv6_prefixd(void);
+static void destroy_ipv6_prefixd(void);
+static void ipv6_prefixd_wait(long long int timeout);
+static void
+prepare_ipv6_prefixd(struct ovsdb_idl_txn *ovnsb_idl_txn,
+ struct ovsdb_idl_index *sbrec_port_binding_by_name,
+ const struct hmap *local_datapaths,
+ const struct sbrec_chassis *chassis,
+ const struct sset *active_tunnels)
+OVS_REQUIRES(pinctrl_mutex);
+static void
+send_ipv6_prefixd(struct rconn *swconn, long long int *send_prefixd_time)
+OVS_REQUIRES(pinctrl_mutex);
static bool may_inject_pkts(void);
static void init_put_vport_bindings(void);
@@ -313,6 +326,10 @@ static void pinctrl_compose_ipv6(struct dp_packet *packet,
uint8_t ip_proto, uint8_t ttl,
uint16_t ip_payload_len);
+static void
+put_load(uint64_t value, enum mf_field_id dst, int ofs, int n_bits,
+ struct ofpbuf *ofpacts);
+
COVERAGE_DEFINE(pinctrl_drop_put_mac_binding);
COVERAGE_DEFINE(pinctrl_drop_buffered_packets_map);
COVERAGE_DEFINE(pinctrl_drop_controller_event);
@@ -470,6 +487,7 @@ pinctrl_init(void)
init_put_mac_bindings();
init_send_garps_rarps();
init_ipv6_ras();
+init_ipv6_prefixd();
init_buffered_packets_map();
init_event_table();
ip_mcast_snoop_init();
@@ -557,6 +575,49 @@ set_actions_and_enqueue_msg(struct rconn *swconn,
ofpbuf_uninit(&ofpacts);
}
+static struct shash ipv6_prefixd;
+
+enum {
+PREFIX_SOLICIT,
+PREFIX_PENDING,
+PREFIX_DONE,
+};
+
+struct ipv6_prefixd_state {
+long long int next_announce;
+struct in6_addr ipv6_addr;
+struct eth_addr ea;
+struct eth_addr cmac;
+int64_t port_key;
+int64_t metadata;
+struct in6_addr prefix;
+unsigned plife_time;
+unsigned vlife_time;
+unsigned aid;
+unsigned t1;
+unsigned t2;
+unsigned plen;
+int state;
+};
+
+static void
+init_ipv6_prefixd(void)
+{
+shash_init(&ipv6_prefixd);
+}
+
+static void
+destroy_ipv6_prefixd(void)
+{
+struct shash_node *iter, *next;
+SHASH_FOR_EACH_SAFE (iter, next, &ipv6_prefixd) {
+struct ipv6_prefixd_state *pfd = iter->data;
+free(pfd);
+shash_delete(&ipv6_prefixd, iter);
+}
+shash_destroy(&ipv6_prefixd);
+}
+
struct buffer_info {
struct ofpbuf ofpacts;
ofp_port_t ofp_port;
@@ -972,6 +1033,277 @@ is_dhcp_flags_broadcast(ovs_be16 flags)
return flags & htons(DHCP_BROADCAST_FLAG);
}
+static void
+pinctrl_parse_dhcpv6_advt(struct rconn *swconn, const struct flow *ip_flow,
+ struct dp_packet *pkt_in, const struct match *md)
+{
+struct udp_header *udp_in = dp_packet_l4(pkt_in);
+size_t dlen = MIN(ntohs(udp_in->udp_len), dp_packet_l4_size(pkt_in));
+unsigned char *in_dhcpv6_data = (unsigned char *)(udp_in + 1);
+uint8_t *data, *end = (uint8_t *)udp_in + dlen;
+int len = 0;
+
+data = xmalloc(dlen);
+/* skip DHCPv6 common header */
+in_dhcpv6_data += 4;
+while (in_dhcpv6_data < end) {
+struct dhcpv6_opt_header *in_opt =
+ (struct dhcpv6_opt_header *)in_dhcpv6_data;
+int opt_len = sizeof *in_opt + ntohs(in_opt->len);
+
+if (dlen < opt_len + len) {
+goto out;
+}
+
+switch (ntohs(in_opt->code)) {
+case DHCPV6_OPT_IA_PD: {
+int orig_len = len, hdr_len
Re: [ovs-dev] [PATCH v2] conntrack: Reset ct_state when entering a new zone.
On 3/5/20 12:28 PM, Dumitru Ceara wrote:
> When a new conntrack zone is entered, the ct_state field is zeroed in
> order to avoid using state information from different zones.
>
> One such scenario is when a packet is double NATed. Assuming two zones
> and 3 flows performing the following actions in order on the packet:
> 1. ct(zone=5,nat), recirc
> 2. ct(zone=1), recirc
> 3. ct(zone=1,nat)
>
> If at step #1 the packet matches an existing NAT entry, it will get
> translated and pkt->md.ct_state is set to CS_DST_NAT or CS_SRC_NAT.
> At step #2 the new tuple might match an existing connection and
> pkt->md.ct_zone is set to 1.
> If at step #3 the packet matches an existing NAT entry in zone 1,
> handle_nat() will be called to perform the translation but it will
> return early because the packet's zone matches the conntrack zone and
> the ct_state field still contains CS_DST_NAT or CS_SRC_NAT from the
> translations in zone 5.
>
> In order to reliably detect when a packet enters a new conntrack zone
> we also need to make sure that the pkt->md.ct_zone is properly
> initialized if pkt->md.ct_state is non-zero. This already happens for
> most cases. The only exception is when matched conntrack connection is
> of type CT_CONN_TYPE_UN_NAT and the master connection is missing. To
> cover this path we now call write_ct_md() in that case too. Remove
> setting the CS_TRACKED flag as in this case as it will be done by the
> new call to write_ct_md().
>
> Also, the error path above seems hard to hit as it would've caused a
> crash due to dereferencing a NULL pointer when calling:
> 'ct_print_conn_info(conn, log_msg, VLL_INFO, true, true)'. This patch
> changes the call to log the 'rev_conn' info instead.
>
> CC: Darrell Ball
> Fixes: 967bb5c5cd90 ("conntrack: Add rcu support.")
> Signed-off-by: Dumitru Ceara
>
> ---
> V2:
> - Address Ilya's comments:
> - revert changes to pkt_metadata_init().
> - update ct_state in process_one() only if ct_state is already
> non-zero.
> - Make sure pkt->md.ct_zone is always initialized when pkt->md.ct_state
> is non-zero.
> - Fix NULL pointer dereference in process_one() if conn_type is
> CT_CONN_TYPE_UN_NAT and master conn is not found.
> ---
'Fixes' tag seems a bit strange to me. I think it should be:
Fixes: 286de2729955 ("dpdk: Userspace Datapath: Introduce NAT Support.")
Regarding the issue. I've spent some time exploring the conntrack code
and also researching the original patch that introduced this code.
The issue was raised during the review of the original patch 286de2729955
by Daniele: https://patchwork.ozlabs.org/patch/743108/
And Darrell said that he actually had the similar code that clears ct_state
during zone transition at the beginning of process_one(). But, he decided
that most of such issues are likely configuration bugs that should by raised
to user with warnings.
However, such warnings was never introduced and since this causes a real
issue in OVN we should actually have this clearing of conntrack state on
zone transitioning.
Acked-by: Ilya Maximets
Darrell, Ben, I'd like to have some comments on this from you since I'm
not an expert in this area. Otherwise, I'm going to apply this patch on
next week.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] 答复: [PATCH v7] Use TPACKET_V3 to accelerate veth for userspace datapath
Ilya, raw socket for the interface type of which is "system" has been set to non-block mode, can you explain which syscall will lead to sleep? Yes, pmd thread will consume CPU resource even if it has nothing to do, but all the type=dpdk ports are handled by pmd thread, here we just let system interfaces look like a DPDK interface. I didn't see any problem in my test, it will be better if you can tell me what will result in a problem and how I can reproduce it. By the way, type=tap/internal interfaces are still be handled by ovs-vswitchd thread. In addition, only one line change is there, ".is_pmd = true,", ".is_pmd = false," will keep it in ovs-vswitchd if there is any other concern. We can change non-thread-safe parts to support pmd. -邮件原件- 发件人: dev [mailto:ovs-dev-boun...@openvswitch.org] 代表 Ilya Maximets 发送时间: 2020年3月18日 19:45 收件人: yang_y...@163.com; ovs-dev@openvswitch.org 抄送: i.maxim...@ovn.org 主题: Re: [ovs-dev] [PATCH v7] Use TPACKET_V3 to accelerate veth for userspace datapath On 3/18/20 10:02 AM, yang_y...@163.com wrote: > From: Yi Yang > > We can avoid high system call overhead by using TPACKET_V3 and using > DPDK-like poll to receive and send packets (Note: send still needs to > call sendto to trigger final packet transmission). > > From Linux kernel 3.10 on, TPACKET_V3 has been supported, so all the > Linux kernels current OVS supports can run > TPACKET_V3 without any problem. > > I can see about 50% performance improvement for veth compared to last > recvmmsg optimization if I use TPACKET_V3, it is about 2.21 Gbps, but > it was 1.47 Gbps before. > > After is_pmd is set to true, performance can be improved much more, it > is about 180% performance improvement. > > TPACKET_V3 can support TSO, but its performance isn't good because of > TPACKET_V3 kernel implementation issue, so it falls back to recvmmsg > in case userspace-tso-enable is set to true, but its performance is > better than recvmmsg in case userspace-tso-enable is set to false, so > just use TPACKET_V3 in that case. > > Note: how much performance improvement is up to your platform, some > platforms can see huge improvement, some ones aren't so noticeable, > but if is_pmd is set to true, you can see big performance improvement, > the prerequisite is your tested veth interfaces should be attached to > different pmd threads. > > Signed-off-by: Yi Yang > Co-authored-by: William Tu > Signed-off-by: William Tu > --- > acinclude.m4 | 12 ++ > configure.ac | 1 + > include/sparse/linux/if_packet.h | 111 +++ > lib/dp-packet.c | 18 ++ > lib/dp-packet.h | 9 + > lib/netdev-linux-private.h | 26 +++ > lib/netdev-linux.c | 419 +-- > 7 files changed, 579 insertions(+), 17 deletions(-) > > Changelog: > - v6->v7 > * is_pmd is set to true for system interfaces This can not be done that simple and should not be done unconditionally anyways. netdev-linux is not thread safe in many ways. At least, stats accounting will be messed up. Second thing is that this change will harm all the usual DPDK-based setups since PMD threads will start make a lot of syscalls and sleep inside the kernel missing packets from the fast DPDK interfaces. Third thing is that this change will fire up at least one PMD thread consuming 100% CPU constantly even for setups where it's not needed. So, this version is definitely not acceptable. Best regards, Ilya Maximets. ___ 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] conntrack: Fix NULL pointer dereference.
On 3/18/20 12:12 AM, William Tu wrote:
> Coverity CID 279957 reports NULL pointer derefence when
> 'conn' is NULL and calling ct_print_conn_info.
>
> Cc: Usman Ansari
> Signed-off-by: William Tu
> ---
> lib/conntrack.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index ff5a89457c0a..001a37ff6aff 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -1302,7 +1302,7 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
> if (!conn) {
> pkt->md.ct_state |= CS_TRACKED | CS_INVALID;
> char *log_msg = xasprintf("Missing master conn %p",
> rev_conn);
> -ct_print_conn_info(conn, log_msg, VLL_INFO, true, true);
> +ct_print_conn_info(rev_conn, log_msg, VLL_INFO, true, true);
> free(log_msg);
> return;
> }
>
Hi.
This issue is addressed as part of the following patch:
https://patchwork.ozlabs.org/patch/1249513/
I'm not sure if we need to split it and fix this issue separately.
Thoughts?
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v7] Use TPACKET_V3 to accelerate veth for userspace datapath
On 3/18/20 10:02 AM, yang_y...@163.com wrote: > From: Yi Yang > > We can avoid high system call overhead by using TPACKET_V3 > and using DPDK-like poll to receive and send packets (Note: send > still needs to call sendto to trigger final packet transmission). > > From Linux kernel 3.10 on, TPACKET_V3 has been supported, > so all the Linux kernels current OVS supports can run > TPACKET_V3 without any problem. > > I can see about 50% performance improvement for veth compared to > last recvmmsg optimization if I use TPACKET_V3, it is about 2.21 > Gbps, but it was 1.47 Gbps before. > > After is_pmd is set to true, performance can be improved much > more, it is about 180% performance improvement. > > TPACKET_V3 can support TSO, but its performance isn't good because > of TPACKET_V3 kernel implementation issue, so it falls back to > recvmmsg in case userspace-tso-enable is set to true, but its > performance is better than recvmmsg in case userspace-tso-enable is > set to false, so just use TPACKET_V3 in that case. > > Note: how much performance improvement is up to your platform, > some platforms can see huge improvement, some ones aren't so > noticeable, but if is_pmd is set to true, you can see big > performance improvement, the prerequisite is your tested veth > interfaces should be attached to different pmd threads. > > Signed-off-by: Yi Yang > Co-authored-by: William Tu > Signed-off-by: William Tu > --- > acinclude.m4 | 12 ++ > configure.ac | 1 + > include/sparse/linux/if_packet.h | 111 +++ > lib/dp-packet.c | 18 ++ > lib/dp-packet.h | 9 + > lib/netdev-linux-private.h | 26 +++ > lib/netdev-linux.c | 419 > +-- > 7 files changed, 579 insertions(+), 17 deletions(-) > > Changelog: > - v6->v7 > * is_pmd is set to true for system interfaces This can not be done that simple and should not be done unconditionally anyways. netdev-linux is not thread safe in many ways. At least, stats accounting will be messed up. Second thing is that this change will harm all the usual DPDK-based setups since PMD threads will start make a lot of syscalls and sleep inside the kernel missing packets from the fast DPDK interfaces. Third thing is that this change will fire up at least one PMD thread consuming 100% CPU constantly even for setups where it's not needed. So, this version is definitely not acceptable. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v7] Use TPACKET_V3 to accelerate veth for userspace datapath
From: Yi Yang
We can avoid high system call overhead by using TPACKET_V3
and using DPDK-like poll to receive and send packets (Note: send
still needs to call sendto to trigger final packet transmission).
>From Linux kernel 3.10 on, TPACKET_V3 has been supported,
so all the Linux kernels current OVS supports can run
TPACKET_V3 without any problem.
I can see about 50% performance improvement for veth compared to
last recvmmsg optimization if I use TPACKET_V3, it is about 2.21
Gbps, but it was 1.47 Gbps before.
After is_pmd is set to true, performance can be improved much
more, it is about 180% performance improvement.
TPACKET_V3 can support TSO, but its performance isn't good because
of TPACKET_V3 kernel implementation issue, so it falls back to
recvmmsg in case userspace-tso-enable is set to true, but its
performance is better than recvmmsg in case userspace-tso-enable is
set to false, so just use TPACKET_V3 in that case.
Note: how much performance improvement is up to your platform,
some platforms can see huge improvement, some ones aren't so
noticeable, but if is_pmd is set to true, you can see big
performance improvement, the prerequisite is your tested veth
interfaces should be attached to different pmd threads.
Signed-off-by: Yi Yang
Co-authored-by: William Tu
Signed-off-by: William Tu
---
acinclude.m4 | 12 ++
configure.ac | 1 +
include/sparse/linux/if_packet.h | 111 +++
lib/dp-packet.c | 18 ++
lib/dp-packet.h | 9 +
lib/netdev-linux-private.h | 26 +++
lib/netdev-linux.c | 419 +--
7 files changed, 579 insertions(+), 17 deletions(-)
Changelog:
- v6->v7
* is_pmd is set to true for system interfaces
* Use zero copy for tpacket_v3 receiving
* Fix comments by William
* Remove include/linux/if_packet.h
- v5->v6
* Fall back to recvmmsg in case userspace-tso-enable is true
because of TPACKET_V3 kernel implementation issue for tso
support
- v4->v5
* Fix travis build issues
* Fix comments issues (capitalize the first letter)
* Verify TSO on Ubuntu 18.04 3.5.0-40-generic
- v3->v4
* Fix sparse check errors
- v2->v3
* Fix build issues in case HAVE_TPACKET_V3 is not defined
* Add tso-related support code
* make sure it can work normally in case userspace-tso-enable is true
- v1->v2
* Remove TPACKET_V1 and TPACKET_V2 which is obsolete
* Add include/linux/if_packet.h
* Change include/sparse/linux/if_packet.h
diff --git a/acinclude.m4 b/acinclude.m4
index 02efea6..1488ded 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -1082,6 +1082,18 @@ AC_DEFUN([OVS_CHECK_IF_DL],
AC_SEARCH_LIBS([pcap_open_live], [pcap])
fi])
+dnl OVS_CHECK_LINUX_TPACKET
+dnl
+dnl Configure Linux TPACKET.
+AC_DEFUN([OVS_CHECK_LINUX_TPACKET], [
+ AC_COMPILE_IFELSE([
+AC_LANG_PROGRAM([#include ], [
+struct tpacket3_hdr x = { 0 };
+])],
+[AC_DEFINE([HAVE_TPACKET_V3], [1],
+[Define to 1 if struct tpacket3_hdr is available.])])
+])
+
dnl Checks for buggy strtok_r.
dnl
dnl Some versions of glibc 2.7 has a bug in strtok_r when compiling
diff --git a/configure.ac b/configure.ac
index 1877aae..b61a1f4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -89,6 +89,7 @@ OVS_CHECK_VISUAL_STUDIO_DDK
OVS_CHECK_COVERAGE
OVS_CHECK_NDEBUG
OVS_CHECK_NETLINK
+OVS_CHECK_LINUX_TPACKET
OVS_CHECK_OPENSSL
OVS_CHECK_LIBCAPNG
OVS_CHECK_LOGDIR
diff --git a/include/sparse/linux/if_packet.h b/include/sparse/linux/if_packet.h
index 5ff6d47..0ac3fce 100644
--- a/include/sparse/linux/if_packet.h
+++ b/include/sparse/linux/if_packet.h
@@ -5,6 +5,7 @@
#error "Use this header only with sparse. It is not a correct implementation."
#endif
+#include
#include_next
/* Fix endianness of 'spkt_protocol' and 'sll_protocol' members. */
@@ -27,4 +28,114 @@ struct sockaddr_ll {
unsigned char sll_addr[8];
};
+/* Packet types */
+#define PACKET_HOST 0 /* To us*/
+#define PACKET_OTHERHOST3 /* To someone else */
+#define PACKET_LOOPBACK 5 /* MC/BRD frame looped back */
+
+/* Packet socket options */
+#define PACKET_RX_RING 5
+#define PACKET_VERSION 10
+#define PACKET_TX_RING 13
+#define PACKET_VNET_HDR15
+
+/* Rx ring - header status */
+#define TP_STATUS_KERNEL0
+#define TP_STATUS_USER(1 << 0)
+#define TP_STATUS_VLAN_VALID (1 << 4) /* auxdata has valid tp_vlan_tci */
+#define TP_STATUS_VLAN_TPID_VALID (1 << 6) /* auxdata has valid tp_vlan_tpid */
+
+/* Tx ring - header status */
+#define TP_STATUS_SEND_REQUEST(1 << 0)
+#define TP_STATUS_SENDING (1 << 1)
+
+#define tpacket_hdr rpl_tpacket_hdr
+struct tpacket_hdr {
+unsigned long tp_status;
+unsigned int tp_len;
+unsigned int tp_snaplen;
+unsigned short tp_mac;
+unsigned short tp_net;
+unsigned
