On Mon, Oct 21, 2013 at 01:59:12PM -0700, Ben Pfaff wrote:
> On Tue, Oct 15, 2013 at 05:17:48PM +0900, Simon Horman wrote:
> > This is a first step towards implementing the dpif side of groups.
> >
> > In order to be useful the action translation code needs
> > to be taught about groups.
> >
> > Signed-off-by: Simon Horman <ho...@verge.net.au>
>
> The code in group_get_stats() worries me. First, what's this? I
> don't see why one would expect to have "struct bucket_counter"s
> following the 'ogs' argument:
> struct bucket_counter *bc = (struct bucket_counter *)(ogs + 1);
Thanks, that is rather bogus. I have fixed
the code to just use ogs->bucket_stats directly.
> Second, if the following comment in group_get_stats() is correct, then
> I don't see how push_all_stats__() helps. If the group that
> group_get_stats() operates on can disappear then the right solution is
> to take some lock or increment some refcount in the caller:
> /* push_all_stats() can handle flow misses which, when using the learn
> * action, can cause groups to be added and deleted. This can corrupt our
> * caller's datastructures which assume that group_get_stats() doesn't
> have
> * an impact on the flow table. To be safe, we disable miss handling. */
> push_all_stats__(false);
I believe that is a copy-paste error as I based the groups code, in part,
on the rules code. I have removed it.
With regards to locking to prevent the group disappearing,
I believe that is handled by up.group->rwlock being held
by the caller.
>
> Here are some tiny style fixes you can fold in:
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index b0c358a..470e455 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -4827,7 +4827,7 @@ group_dealloc(struct ofgroup *group_)
>
> static void
> group_construct_stats(struct group_dpif *group)
> - OVS_REQUIRES(&group->stats_mutex)
> + OVS_REQUIRES(group->stats_mutex)
> {
> group->packet_count = 0;
> group->byte_count = 0;
> @@ -4853,7 +4853,7 @@ group_construct(struct ofgroup *group_)
>
> static void
> group_destruct__(struct group_dpif *group)
> - OVS_REQUIRES(&group->stats_mutex)
> + OVS_REQUIRES(group->stats_mutex)
> {
> free(group->bucket_stats);
> group->bucket_stats = NULL;
> @@ -4885,7 +4885,6 @@ group_modify(struct ofgroup *group_, struct ofgroup
> *victim_)
> return 0;
> }
>
> -
> static enum ofperr
> group_get_stats(const struct ofgroup *group_, struct ofputil_group_stats
> *ogs)
> {
>
Thanks, I will include those changes in the next post.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
