[ovs-dev] ofproto-dpif-xlate: Do not use zero-weight buckets in select groups.
diff mbox series

Message ID 20190607181300.28213-1-blp@ovn.org
State New
Headers show
Series
  • [ovs-dev] ofproto-dpif-xlate: Do not use zero-weight buckets in select groups.
Related show

Commit Message

Ben Pfaff June 7, 2019, 6:13 p.m. UTC
The OpenFlow specification says that buckets in select groups with a weight
of zero should not be selected, but the ofproto-dpif implementation could
select them in corner cases.  This fixes the problem.

Reported-by: ychen <ychen103103@163.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2019-May/359349.html
Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 ofproto/ofproto-dpif-xlate.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

0-day Robot June 7, 2019, 7:01 p.m. UTC | #1
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 88 characters long (recommended limit is 79)
#23 FILE: ofproto/ofproto-dpif-xlate.c:1:
/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017, 2019 Nicira, Inc.

Lines checked: 47, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot
Yifeng Sun June 7, 2019, 10:27 p.m. UTC | #2
Hi Ben,

group_first_live_bucket() can still return zero-weighted bucket, then
this bucket will
be used via pick_ff_group() and xlate_group_action__(). I am wondering
if it is an
issue?

Thanks,
Yifeng

On Fri, Jun 7, 2019 at 12:04 PM 0-day Robot <robot@bytheb.org> wrote:
>
> 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 88 characters long (recommended limit is 79)
> #23 FILE: ofproto/ofproto-dpif-xlate.c:1:
> /* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017, 2019 Nicira, Inc.
>
> Lines checked: 47, Warnings: 1, Errors: 0
>
>
> Please check this out.  If you feel there has been an error, please email aconole@bytheb.org
>
> Thanks,
> 0-day Robot
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ben Pfaff June 7, 2019, 10:55 p.m. UTC | #3
Weights are significant only for "select" groups;
group_first_live_bucket() is used to pick a bucket only for
"fast-failover" groups.  So I think that this is correct for group
selection now.

However, group_is_alive() also uses group_first_live_bucket() for
determining whether a group is alive, and for that purpose it should
consider the group type whereas currently it just treats every group as
if it was a fast-failover group.

I'll fix that and send a v2.

On Fri, Jun 07, 2019 at 03:27:28PM -0700, Yifeng Sun wrote:
> Hi Ben,
> 
> group_first_live_bucket() can still return zero-weighted bucket, then
> this bucket will
> be used via pick_ff_group() and xlate_group_action__(). I am wondering
> if it is an
> issue?
> 
> Thanks,
> Yifeng
> 
> On Fri, Jun 7, 2019 at 12:04 PM 0-day Robot <robot@bytheb.org> wrote:
> >
> > 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 88 characters long (recommended limit is 79)
> > #23 FILE: ofproto/ofproto-dpif-xlate.c:1:
> > /* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017, 2019 Nicira, Inc.
> >
> > Lines checked: 47, Warnings: 1, Errors: 0
> >
> >
> > Please check this out.  If you feel there has been an error, please email aconole@bytheb.org
> >
> > Thanks,
> > 0-day Robot
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ben Pfaff June 7, 2019, 11:30 p.m. UTC | #4
I sent v2:
https://mail.openvswitch.org/pipermail/ovs-dev/2019-June/359582.html

On Fri, Jun 07, 2019 at 03:55:36PM -0700, Ben Pfaff wrote:
> Weights are significant only for "select" groups;
> group_first_live_bucket() is used to pick a bucket only for
> "fast-failover" groups.  So I think that this is correct for group
> selection now.
> 
> However, group_is_alive() also uses group_first_live_bucket() for
> determining whether a group is alive, and for that purpose it should
> consider the group type whereas currently it just treats every group as
> if it was a fast-failover group.
> 
> I'll fix that and send a v2.
> 
> On Fri, Jun 07, 2019 at 03:27:28PM -0700, Yifeng Sun wrote:
> > Hi Ben,
> > 
> > group_first_live_bucket() can still return zero-weighted bucket, then
> > this bucket will
> > be used via pick_ff_group() and xlate_group_action__(). I am wondering
> > if it is an
> > issue?
> > 
> > Thanks,
> > Yifeng
> > 
> > On Fri, Jun 7, 2019 at 12:04 PM 0-day Robot <robot@bytheb.org> wrote:
> > >
> > > 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 88 characters long (recommended limit is 79)
> > > #23 FILE: ofproto/ofproto-dpif-xlate.c:1:
> > > /* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017, 2019 Nicira, Inc.
> > >
> > > Lines checked: 47, Warnings: 1, Errors: 0
> > >
> > >
> > > Please check this out.  If you feel there has been an error, please email aconole@bytheb.org
> > >
> > > Thanks,
> > > 0-day Robot
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Patch
diff mbox series

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index ae8b9991c7cd..da7fa56362d3 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1,4 +1,4 @@ 
-/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017 Nicira, Inc.
+/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017, 2019 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -1929,7 +1929,7 @@  group_best_live_bucket(const struct xlate_ctx *ctx,
 
     struct ofputil_bucket *bucket;
     LIST_FOR_EACH (bucket, list_node, &group->up.buckets) {
-        if (bucket_is_alive(ctx, bucket, 0)) {
+        if (bucket_is_alive(ctx, bucket, 0) && bucket->weight > 0) {
             uint32_t score =
                 (hash_int(bucket->bucket_id, basis) & 0xffff) * bucket->weight;
             if (score >= best_score) {
@@ -4535,7 +4535,7 @@  pick_dp_hash_select_group(struct xlate_ctx *ctx, struct group_dpif *group)
         for (int i = 0; i <= hash_mask; i++) {
             struct ofputil_bucket *b =
                     group->hash_map[(dp_hash + i) & hash_mask];
-            if (bucket_is_alive(ctx, b, 0)) {
+            if (bucket_is_alive(ctx, b, 0) && b->weight > 0) {
                 return b;
             }
         }