Ranko Zivojnovic wrote:
> On Sat, 2007-07-07 at 17:10 +0200, Patrick McHardy wrote:
>
>>On Sat, 7 Jul 2007, Ranko Zivojnovic wrote:
>>
>>>Maybe the appropriate way to fix this would to call gen_kill_estimator,
>>>with the appropriate lock order, before the call to qdisc_destroy, so
>>>when dev->queue_lock is taken for qdisc_destroy - the structure is
>>>already off the list.
>>
>>Probably easier to just kill est_lock and use rcu lists.
>>I'm currently travelling, I'll look into it tomorrow.
>
>
> 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.
Thanks for taking care of this, I'm a bit behind.
See below for comments ..
> --- a/net/core/gen_estimator.c 2007-06-25 02:21:48.000000000 +0300
> +++ b/net/core/gen_estimator.c 2007-07-09 14:27:12.053336875 +0300
> @@ -79,7 +79,7 @@
>
> struct gen_estimator
> {
> - struct gen_estimator *next;
> + struct list_head list;
> struct gnet_stats_basic *bstats;
> struct gnet_stats_rate_est *rate_est;
> spinlock_t *stats_lock;
> @@ -89,26 +89,27 @@
> u32 last_packets;
> u32 avpps;
> u32 avbps;
> + struct rcu_head e_rcu;
You could also use synchronize_rcu(), estimator destruction is
not particulary performance critical.
> static struct gen_estimator_head elist[EST_MAX_INTERVAL+1];
>
> /* Estimator array lock */
> -static DEFINE_RWLOCK(est_lock);
> +static DEFINE_SPINLOCK(est_lock);
The lock isn't needed anymore since we can rely on the rtnl
for creation/destruction.
> /**
> @@ -152,6 +153,7 @@
> {
> struct gen_estimator *est;
> struct gnet_estimator *parm = RTA_DATA(opt);
> + int idx;
>
> if (RTA_PAYLOAD(opt) < sizeof(*parm))
> return -EINVAL;
> @@ -163,7 +165,8 @@
> if (est == NULL)
> return -ENOBUFS;
>
> - est->interval = parm->interval + 2;
> + INIT_LIST_HEAD(&est->list);
Initializing the members' list_head isn't necessary.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
