[ovs-dev] [PATCH v7 1/3] ofproto-dpif: Implement group callbacks

[Simon Horman]

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 <horms at 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.