On 9/9/20 20:34, Ilya Maximets wrote:
> On 6/8/19 1:28 AM, Ben Pfaff wrote:
>> 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 <[email protected]>
>> Reported-at:
>> https://mail.openvswitch.org/pipermail/ovs-dev/2019-May/359349.html
>> Signed-off-by: Ben Pfaff <[email protected]>
>> ---
>> v1->v2: Simplify and fix figuring out whether a group is alive.
>
> Hi, Ben. I'm looking through old patches after the patchwork cleanup and this
> one seems to be still valid. It requires a minor rebase, but it's trivial.
>
> Patch looks good to me in general and I could apply it.
> What do you think?
>
> It also looks like we will need to backport it. Suggestions on how far it
> should go are welcome.
Third year's a charm. :)
Looking again through the oldest patches in a patchwork, this still
seems correct to me. So, applied now. Also backported down to 2.17.
Best regards, Ilya Maximets.
>
> Best regards, Ilya Maximets.
>
>>
>> ofproto/ofproto-dpif-xlate.c | 18 ++++++++++++------
>> 1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index 04d69ed06c20..1ace92d22019 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.
>> @@ -1864,8 +1864,8 @@ group_is_alive(const struct xlate_ctx *ctx, uint32_t
>> group_id, int depth)
>> #define MAX_LIVENESS_RECURSION 128 /* Arbitrary limit */
>>
>> static bool
>> -bucket_is_alive(const struct xlate_ctx *ctx,
>> - struct ofputil_bucket *bucket, int depth)
>> +bucket_is_alive(const struct xlate_ctx *ctx, const struct group_dpif *group,
>> + const struct ofputil_bucket *bucket, int depth)
>> {
>> if (depth >= MAX_LIVENESS_RECURSION) {
>> xlate_report_error(ctx, "bucket chaining exceeded %d links",
>> @@ -1873,6 +1873,12 @@ bucket_is_alive(const struct xlate_ctx *ctx,
>> return false;
>> }
>>
>> + /* In "select" groups, buckets with weight 0 are not used.
>> + * In other kinds of groups, weight does not matter. */
>> + if (group->up.type == OFPGT11_SELECT && bucket->weight == 0) {
>> + return false;
>> + }
>> +
>> return (!ofputil_bucket_has_liveness(bucket)
>> || (bucket->watch_port != OFPP_ANY
>> && odp_port_is_alive(ctx, bucket->watch_port))
>> @@ -1910,7 +1916,7 @@ group_first_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, depth)) {
>> + if (bucket_is_alive(ctx, group, bucket, depth)) {
>> return bucket;
>> }
>> xlate_report_bucket_not_live(ctx, bucket);
>> @@ -1929,7 +1935,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, group, bucket, 0)) {
>> uint32_t score =
>> (hash_int(bucket->bucket_id, basis) & 0xffff) *
>> bucket->weight;
>> if (score >= best_score) {
>> @@ -4535,7 +4541,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, group, b, 0)) {
>> return b;
>> }
>> }
>>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
