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.solo...@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@entry=0x55cab8fd6cd0 <append_group_stats>) at > ofproto/ofproto.c:6790 > > Signed-off-by: solomon <liwei.solo...@gmail.com> > Signed-off-by: Ben Pfaff <b...@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)]) > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev