Re: [PATCH] part 3 - Convert IPMI driver over to use refcounts

[Corey Minyard]

Andrew Morton wrote:

> > The IPMI driver uses read/write locks to ensure that things
> > exist while they are in use.  This is bad from a number of
> > points of view.  This patch removes the rwlocks and uses
> > refcounts and a special synced list (the entries can be
> > refcounted and removal is blocked while an entry is in
> > use).
> >
> >    
>
> This:
>
>  
>
> > +
> > +struct synced_list_entry_task_q
> > {
> > 	struct list_head link;
> > +	task_t           *process;
> > +};
> > +
> >    
>
>  
>
> > +#define synced_list_for_each_entry(pos, l, entry, flags)		\
> > +	for ((spin_lock_irqsave(&(l)->lock, flags),			      \
> > +	      pos = container_of((l)->head.next, typeof(*(pos)),entry.link)); \
> > +	     (prefetch((pos)->entry.link.next),				      \
> > +	      &(pos)->entry.link != (&(l)->head)			      \
> > +	        ? (atomic_inc(&(pos)->entry.usecount),			      \
> > +                   spin_unlock_irqrestore(&(l)->lock, flags), 1)	      \
> > +	        : (spin_unlock_irqrestore(&(l)->lock, flags), 0));	      \
> > +	     (spin_lock_irqsave(&(l)->lock, flags),			      \
> > +	      synced_list_wake(&(pos)->entry),				      \
> > +              pos = container_of((pos)->entry.link.next, typeof(*(pos)),      \
> > +				 entry.link)))
> >    
>
> (gad)
>  
Yes, I was trying to preserve list semantics and it got out of hand. It 
is pretty ugly.

> And this:
>
>  
>
> > +static int synced_list_clear(struct synced_list *head,
> > +			     int (*match)(struct synced_list_entry *,
> > +					  void *),
> > +			     void (*free)(struct synced_list_entry *),
> > +			     void *match_data)
> > +{
> > +	struct synced_list_entry *ent, *ent2;
> > +	int                      rv = -ENODEV;
> > +	int                      mrv = SYNCED_LIST_MATCH_CONTINUE;
> > +
> > +	spin_lock_irq(&head->lock);
> > + restart:
> > +	list_for_each_entry_safe(ent, ent2, &head->head, link) {
> > +		if (match) {
> > +			mrv = match(ent, match_data);
> > +			if (mrv == SYNCED_LIST_NO_MATCH)
> > +				continue;
> > +		}
> > +		if (atomic_read(&ent->usecount)) {
> > +			struct synced_list_entry_task_q e;
> > +			e.process = current;
> > +			list_add(&e.link, &ent->task_list);
> > +			__set_current_state(TASK_UNINTERRUPTIBLE);
> > +			spin_unlock_irq(&head->lock);
> > +			schedule();
> > +			spin_lock_irq(&head->lock);
> > +			list_del(&e.link);
> > +			goto restart;
> > +		}
> > +		list_del(&ent->link);
> > +		rv = 0;
> > +		if (free)
> > +			free(ent);
> > +		if (mrv == SYNCED_LIST_MATCH_STOP)
> > +			break;
> > +	}
> > +	spin_unlock_irq(&head->lock);
> > +	return rv;
> > +}
> >    
>
> Look awfully similar to wait_queue_head_t.  Are you sure existing
> infrastructure cannot be used?
>  
I already had a lock and it seemed wasteful to have two locks. And it 
didn't really save much code and it didn't quite fit.

Basically, I need a notifier list that avoids traversal/removal race 
conditions. An asynchronous event or command comes in and you need to 
inform all the users who are waiting for events or commands. I don't 
think anything like that exists currently (the current notifier list has 
no lock on traversal). I don't want a lock around the whole traversal 
because doing callbacks with locks held is not a good idea.

The list I created simply blocks a deleter while the item is in use. 
There may be a better way to handle this, I'll think about it (and take 
any suggestions :). I read some stuff about RCU when looking at this, 
and it didn't seem suitable at the time, but I just reread it and it 
seems like it would be fine. I'll remove all this garbage and replace it 
with an RCU list.

>  
>
> > 1 files changed, 692 insertions(+), 461 deletions(-)
> >    
>
> Ow.  Why is it worthwhile?
>  
The change is definately needed. The driver, as it currently is, holds 
read locks for very long periods of time. It does this to make sure 
various objects don't get deleted. Holding read locks for this long is 
bad, and this is the job of refcounts. It's hard to break up changing 
fundamental underpinnings like this.

The patch is big, and from the above it looks like it is not quite 
ready. I'll continue to look at it.

Thank you,

-Corey
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/