[ovs-dev] [PATCH] ovs: fix the bug of bucket counter is not updated
solomon
shanwei88 at gmail.com
Thu Mar 21 02:41:05 UTC 2019
Ben Pfaff wrote:
> On Wed, Mar 20, 2019 at 08:16:18PM +0800, Li Wei wrote:
>>
>> After inserting/removing a bucket, we don't update the bucket counter.
>> When we call ovs-ofctl dump-group-stats br-int, a panic happened.
>
> Thanks for the patch! It looks correct to me. Thank you for adding a
> test, too.
>
> I took a closer look and I saw that 'n_buckets' is not very useful,
> because it is only used in cases where the code is already
> O(n_buckets). I think that we can just remove it. Then it cannot get
> out-of-sync. What do you think of this variation of your patch?
ovs_list_size() will traversing the list to get the total length.
In our custom scheduling algorithms (eg wrr, least-connection),
we need to know the total number of buckets before traversing the bucket list to hit target bucket.
so, it is traversed twice.
If the number of buckets reaches 100+, there are tens of thousands of groups, don't this modification affect performance?
I hope to keep n_buckets in struct ofgroup.
>
> --8<--------------------------cut here-------------------------->8--
>
> From: Li Wei <liwei.solomon at gmail.com>
> Date: Wed, 20 Mar 2019 20:16:18 +0800
> Subject: [PATCH] ofproto: fix the bug of bucket counter is not updated
>
> After inserting/removing a bucket, we don't update the bucket counter.
> When we call ovs-ofctl dump-group-stats br-int, a panic happened.
>
> Reproduce steps:
> 1. ovs-ofctl -O OpenFlow15 add-group br-int "group_id=1, type=select, selection_method=hash bucket=bucket_id=1,weight:100,actions=output:1"
> 2. ovs-ofctl insert-buckets br-int "group_id=1, command_bucket_id=last, bucket=bucket_id=7,weight:800,actions=output:1"
> 3. ovs-ofctl dump-group-stats br-int
>
> gdb) bt
> at ../sysdeps/posix/libc_fatal.c:175
> ar_ptr=<optimized out>) at malloc.c:5049
> group_id=<optimized out>, cb=cb at entry=0x55cab8fd6cd0 <append_group_stats>) at ofproto/ofproto.c:6790
>
> Signed-off-by: solomon <liwei.solomon at gmail.com>
> Signed-off-by: Ben Pfaff <blp at ovn.org>
> ---
> ofproto/ofproto-dpif.c | 2 +-
> ofproto/ofproto-provider.h | 1 -
> ofproto/ofproto.c | 11 ++++-------
> tests/ofproto.at | 16 ++++++++++++++++
> 4 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 21dd54bab09b..cc6dbc70fe41 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -4754,7 +4754,7 @@ static bool
> group_setup_dp_hash_table(struct group_dpif *group, size_t max_hash)
> {
> struct ofputil_bucket *bucket;
> - uint32_t n_buckets = group->up.n_buckets;
> + uint32_t n_buckets = ovs_list_size(&group->up.buckets);
> uint64_t total_weight = 0;
> uint16_t min_weight = UINT16_MAX;
> struct webster {
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index d1a87a59e47e..a71d911199f4 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -573,7 +573,6 @@ struct ofgroup {
> const long long int modified; /* Time of last modification. */
>
> const struct ovs_list buckets; /* Contains "struct ofputil_bucket"s. */
> - const uint32_t n_buckets;
>
> struct ofputil_group_props props;
>
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 40780e2766ab..ff3b8410bf49 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -6979,11 +6979,12 @@ append_group_stats(struct ofgroup *group, struct ovs_list *replies)
> long long int now = time_msec();
> int error;
>
> - ogs.bucket_stats = xmalloc(group->n_buckets * sizeof *ogs.bucket_stats);
> + size_t n_buckets = ovs_list_size(&group->buckets);
> + ogs.bucket_stats = xmalloc(n_buckets * sizeof *ogs.bucket_stats);
>
> /* Provider sets the packet and byte counts, we do the rest. */
> ogs.ref_count = rule_collection_n(&group->rules);
> - ogs.n_buckets = group->n_buckets;
> + ogs.n_buckets = n_buckets;
>
> error = (ofproto->ofproto_class->group_get_stats
> ? ofproto->ofproto_class->group_get_stats(group, &ogs)
> @@ -6991,8 +6992,7 @@ append_group_stats(struct ofgroup *group, struct ovs_list *replies)
> if (error) {
> ogs.packet_count = UINT64_MAX;
> ogs.byte_count = UINT64_MAX;
> - memset(ogs.bucket_stats, 0xff,
> - ogs.n_buckets * sizeof *ogs.bucket_stats);
> + memset(ogs.bucket_stats, 0xff, n_buckets * sizeof *ogs.bucket_stats);
> }
>
> ogs.group_id = group->group_id;
> @@ -7212,9 +7212,6 @@ init_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm,
> &(*ofgroup)->buckets),
> &gm->buckets, NULL);
>
> - *CONST_CAST(uint32_t *, &(*ofgroup)->n_buckets) =
> - ovs_list_size(&(*ofgroup)->buckets);
> -
> ofputil_group_properties_copy(CONST_CAST(struct ofputil_group_props *,
> &(*ofgroup)->props),
> &gm->props);
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index 213fb262360b..a810dd6042fd 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -1223,6 +1223,22 @@ OFPST_GROUP reply (OF1.5):
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +dnl This is used to find that the bucket counter is not updated.
> +AT_SETUP([ofproto - group stats after insert a new bucket (OpenFlow 1.5)])
> +OVS_VSWITCHD_START
> +AT_DATA([groups.txt], [dnl
> +group_id=1234,type=select,selection_method=hash bucket=bucket_id=1,weight:100,actions=output:10
> +])
> +AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn add-groups br0 groups.txt])
> +AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn insert-buckets br0 'group_id=1234, command_bucket_id=last, bucket=bucket_id=2,weight:100,actions=output:10'])
> +AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn dump-group-stats br0 group_id=1234], [0], [stdout])
> +AT_CHECK([strip_xids < stdout | sed 's/duration=[[0-9.]]*s/duration=?s/' | sort], [0], [dnl
> + group_id=1234,duration=?s,ref_count=0,packet_count=0,byte_count=0,bucket0:packet_count=0,byte_count=0,bucket1:packet_count=0,byte_count=0
> +OFPST_GROUP reply (OF1.5):
> +])
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> dnl This found a use-after-free error in bridge destruction in the
> dnl presence of groups.
> AT_SETUP([ofproto - group add then bridge delete (OpenFlow 1.3)])
>
More information about the dev
mailing list