Re: [Lse-tech] [PATCH] RCU subscriber_list Process Notification (pnotify)

On Thu, Sep 29, 2005 at 11:53:28AM -0500, Erik Jacobson wrote:
> My feeling is we shouldn't use this RCU-protected-subscriber-list version 
> of pnotify - see my performance post to follow.  It includes notes about
> sleeping while rcu_read_lock is held, for exmaple.

On a quick look this patch does look bogus to me.

> +
> +/**
> + * pnotify_unsubscribe_rcu - Free up the pnotify_subscriber when RCU is ready
> + * @rcu - the rcu_head to retrieve the pointer to free from
> + *
> + */
> +static void
> +pnotify_unsubscribe_rcu (struct rcu_head *rcu) {
> +	struct pnotify_subscriber *sub = container_of(rcu,
> +	  struct pnotify_subscriber, rcu);
> +
> +	kfree(sub);
> +}
> +
> +/**
> + *
> + */
> +void
> +pnotify_unsubscribe(struct pnotify_subscriber *subscriber)
> +{
> +	atomic_dec(&subscriber->events->refcnt); /* decr the ref cnt on events */
> +	list_del_rcu(&subscriber->entry);
> +	call_rcu(&subscriber->rcu, pnotify_unsubscribe_rcu);
> +}


Could you use a per-subscriber reference count ? That will allow
you to drop rcu_read_lock() safely.


> +static void
> +remove_subscriber_from_all_tasks(struct pnotify_events *events)
> +{
> +	if (events == NULL)
> +		return;
> +
> +	/* Because of internal race conditions we can't gaurantee
> +	 * getting every task in just one pass so we just keep going
> +	 * until there are no tasks with subscribers from this events struct
> +	 * attached. The inefficiency of this should be tempered by the fact that
> +	 * this happens at most once for each registered client.
> +	 */
> +	while (atomic_read(&events->refcnt) != 0) {
> +		struct task_struct *g = NULL, *p = NULL;
> +
> +		read_lock(&tasklist_lock);
> +		do_each_thread(g, p) {
> +			struct pnotify_subscriber *subscriber;
> +			int task_exited;
> +
> +			get_task_struct(p);
> +			read_unlock(&tasklist_lock);
> +			rcu_read_lock();
> +			down_write(&p->pnotify_subscriber_list_sem);

Wrong. Will refcounting suscrbiber itself here be costly ?

Thanks
Dipankar