From mboxrd@z Thu Jan  1 00:00:00 1970
From: mathieu.desnoyers@efficios.com (Mathieu Desnoyers)
Date: Sat, 5 Nov 2011 10:07:33 -0400
Subject: [ltt-dev] [PATCH 2/2] rculfhash: simplify BUCKET_FLAG tracking
In-Reply-To: <1320392867-9033-2-git-send-email-laijs@cn.fujitsu.com>
References: <466114cca01aa345508567117949e33b57a358bd.1320217693.git.laijs@cn.fujitsu.com>
	<1320392867-9033-1-git-send-email-laijs@cn.fujitsu.com>
	<1320392867-9033-2-git-send-email-laijs@cn.fujitsu.com>
Message-ID: <20111105140733.GH15862@Krystal>

Hi Lai,

* Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
> Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
> ---
>  rculfhash.c |   41 ++++++++++++++++-------------------------
>  1 files changed, 16 insertions(+), 25 deletions(-)
> 
> diff --git a/rculfhash.c b/rculfhash.c
> index 88079f9..0163a1b 100644
> --- a/rculfhash.c
> +++ b/rculfhash.c
> @@ -316,13 +316,6 @@ struct partition_resize_work {
>  		    unsigned long start, unsigned long len);
>  };
>  
> -static
> -void _cds_lfht_add(struct cds_lfht *ht,
> -		unsigned long size,
> -		struct cds_lfht_node *node,
> -		struct cds_lfht_iter *unique_ret,
> -		int bucket);
> -
>  /*
>   * Algorithm to reverse bits in a word by lookup table, extended to
>   * 64-bit words.
> @@ -728,6 +721,14 @@ struct cds_lfht_node *flag_bucket(struct cds_lfht_node *node)
>  }
>  
>  static
> +struct cds_lfht_node *flag_keep_bucket(struct cds_lfht_node *node,
> +		unsigned long bucket)
> +{
> +	return (struct cds_lfht_node *) (((unsigned long) node) |
> +			(bucket & BUCKET_FLAG));
> +}

Hrm. The name is "flag_keep_bucket", but we don't do a clear_flag on
"node" within this helper. So we end up having a behavior that differs
from the helper semantic. I understand that the code below that uses
flag_keep_bucket performs the clear_flag when needed, but I think it
would be good to rename flag_keep_bucket so anyone reviewing the code
would not be mislead by the name: I initially thought that
flag_keep_bucket kept only the "BUCKET" flag and cleared all other flags
from node, which is not the case.

In fact, what it does is to keep the node flag as-is, and OR the BUCKET
flag to node if it is set in the "bucket" argument.

So maybe "bucket_flag_or" would be more appropriate ?

> +
> +static
>  struct cds_lfht_node *get_end(void)
>  {
>  	return (struct cds_lfht_node *) END_VALUE;
> @@ -815,10 +816,7 @@ void _cds_lfht_gc_bucket(struct cds_lfht_node *bucket, struct cds_lfht_node *nod
>  			iter = next;
>  		}
>  		assert(!is_removed(iter));
> -		if (is_bucket(iter))
> -			new_next = flag_bucket(clear_flag(next));
> -		else
> -			new_next = clear_flag(next);
> +		new_next = flag_keep_bucket(clear_flag(next), (unsigned long)iter);


Also please use "(unsigned long) iter" (with a space) to keep the coding
style of the file.

Thanks,

Mathieu


>  		(void) uatomic_cmpxchg(&iter_prev->next, iter, new_next);
>  	}
>  	return;
> @@ -884,13 +882,15 @@ int _cds_lfht_replace(struct cds_lfht *ht, unsigned long size,
>  /*
>   * A non-NULL unique_ret pointer uses the "add unique" (or uniquify) add
>   * mode. A NULL unique_ret allows creation of duplicate keys.
> + *
> + * @bucket_flag should be 0 or BUCKET_FLAG
>   */
>  static
>  void _cds_lfht_add(struct cds_lfht *ht,
>  		unsigned long size,
>  		struct cds_lfht_node *node,
>  		struct cds_lfht_iter *unique_ret,
> -		int bucket_flag)
> +		unsigned long bucket_flag)
>  {
>  	struct cds_lfht_node *iter_prev, *iter, *next, *new_node, *new_next,
>  			*return_node;
> @@ -960,14 +960,8 @@ void _cds_lfht_add(struct cds_lfht *ht,
>  		assert(!is_removed(iter_prev));
>  		assert(!is_removed(iter));
>  		assert(iter_prev != node);
> -		if (!bucket_flag)
> -			node->next = clear_flag(iter);
> -		else
> -			node->next = flag_bucket(clear_flag(iter));
> -		if (is_bucket(iter))
> -			new_node = flag_bucket(node);
> -		else
> -			new_node = node;
> +		node->next = flag_keep_bucket(clear_flag(iter), bucket_flag);
> +		new_node = flag_keep_bucket(node, (unsigned long)iter);
>  		if (uatomic_cmpxchg(&iter_prev->next, iter,
>  				    new_node) != iter) {
>  			continue;	/* retry */
> @@ -978,10 +972,7 @@ void _cds_lfht_add(struct cds_lfht *ht,
>  
>  	gc_node:
>  		assert(!is_removed(iter));
> -		if (is_bucket(iter))
> -			new_next = flag_bucket(clear_flag(next));
> -		else
> -			new_next = clear_flag(next);
> +		new_next = flag_keep_bucket(clear_flag(next), (unsigned long)iter);
>  		(void) uatomic_cmpxchg(&iter_prev->next, iter, new_next);
>  		/* retry */
>  	}
> @@ -1113,7 +1104,7 @@ void init_table_populate_partition(struct cds_lfht *ht, unsigned long i,
>  		new_node->reverse_hash =
>  				bit_reverse_ulong((1UL << (i - 1)) + j);
>  		_cds_lfht_add(ht, 1UL << (i - 1),
> -				new_node, NULL, 1);
> +				new_node, NULL, BUCKET_FLAG);
>  	}
>  	ht->cds_lfht_rcu_read_unlock();
>  }
> -- 
> 1.7.4.4
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com