Re: + gen_estimator-fix-locking-and-timer-related-bugs.patch added to -mm tree

[Jarek Poplawski <jarkao2@o2.pl>]

On Mon, Jul 09, 2007 at 07:43:40PM +0300, Ranko Zivojnovic wrote:
> On Mon, 2007-07-09 at 15:52 +0200, Patrick McHardy wrote:
> > Ranko Zivojnovic wrote:
> > > Patrick, I've taken liberty to try and implement this myself. Attached
> > > is the whole new gen_estimator-fix-locking-and-timer-related-bugs.patch
> > > that is RCU lists based. Please be kind to review.
...

I've some doubts/suggestions too:

> --- a/net/core/gen_estimator.c	2007-06-25 02:21:48.000000000 +0300
> +++ b/net/core/gen_estimator.c	2007-07-09 19:08:06.801544963 +0300
...
> @@ -173,20 +172,24 @@
>  	est->last_packets = bstats->packets;
>  	est->avpps = rate_est->pps<<10;
>  
> -	est->next = elist[est->interval].list;
> -	if (est->next == NULL) {
> -		init_timer(&elist[est->interval].timer);
> -		elist[est->interval].timer.data = est->interval;
> -		elist[est->interval].timer.expires = jiffies + ((HZ<<est->interval)/4);
> -		elist[est->interval].timer.function = est_timer;
> -		add_timer(&elist[est->interval].timer);
> +	if (!elist[idx].timer.function) {
> +		INIT_LIST_HEAD(&elist[idx].list);
> +		setup_timer(&elist[idx].timer, est_timer, est->interval);

s/est->interval/idx/ here and below.

>  	}
> -	write_lock_bh(&est_lock);
> -	elist[est->interval].list = est;
> -	write_unlock_bh(&est_lock);
> +		
> +	if (list_empty(&elist[est->interval].list))
> +		mod_timer(&elist[idx].timer, jiffies + ((HZ<<idx)/4));
> +
> +	list_add_rcu(&est->list, &elist[idx].list);
>  	return 0;
>  }
>  
> +static void __gen_kill_estimator(struct rcu_head *head)
> +{
> +	struct gen_estimator *e = container_of(head, struct gen_estimator, e_rcu);
> +	kfree(e);
> +}
> +
>  /**
>   * gen_kill_estimator - remove a rate estimator
>   * @bstats: basic statistics
> @@ -199,26 +202,21 @@
>  	struct gnet_stats_rate_est *rate_est)
>  {
...
> +		list_for_each_entry_safe(e, n, &elist[idx].list, list) {

IMHO, at least for readability list_for_each_entry_rcu() is better here.

> +			if (e->rate_est != rate_est || e->bstats != bstats)
> +				continue;
>  
> -			kfree(est);
> -			killed++;
> +			list_del_rcu(&e->list);
> +			call_rcu(&e->e_rcu, __gen_kill_estimator);

I think a race is possible here: e.g. a timer could be running
after return from this function yet, and trying to use *bstats,
*rate_est and maybe even stats_lock after their destruction.

BTW, I think, rcu_read_lock/unlock are recommended around e.g.
rcu lists traversals, even if the current implementation doesn't
use them now.

Regards,
Jarek P.