Re: [linux-usb-devel] Re: [PATCH 00/10] usb-serial: Switches from spin lock to atomic_t.

[Eduardo Pereira Habkost]

On Wed, Dec 07, 2005 at 04:22:23PM +0100, Arjan van de Ven wrote:
> 
> > On the other hand, Oliver needs to be careful about claiming too much.  In 
> > general atomic_t operations _are_ superior to the spinlock approach.
> 
> No they're not. Both are just about equally expensive cpu wise,
> sometimes the atomic_t ones are a bit more expensive (like on parisc
> architecture). But on x86 in either case it's a locked cycle, which is
> just expensive no matter which side you flip the coin... 

But if a lock is used exclusively to protect a int variable, an atomic_t
seems to be more appropriate to me. Isn't it?

> 
> >   If 
> > they weren't, atomic_t wouldn't belong in the kernel at all.
> 
> there's different usage patterns where either makes sense. 
> In this case it looks just disgusting on very first sight; the atomic
> are used to implement a lock, and that lock itself is then implemented
> with a spinlock again. For me, again on first sight, the real solution
> appears to be to use a linux primitive for the higher level lock in the
> first place, instead of reimplementing <your own thing> with <another
> own thing>.

The patches don't implement any new lock scheme neither change the
current behaviour. They just replace a spin_lock + integer variable
(the spinlock is used just to protect an integer variable) by an atomic_t.

The patches aren't adding any "own lock scheme", but just replacing
a spinlock that is used exclusively to protect an integer variable
(write_urb_busy) by an atomic_t.

The whole point of the changes is that using a spin_lock to protect only
a int variable doesn't look like a Right Thing.

Please, if you could, review the patches with this in mind: we aren't
changing any behaviour neither creating any weird lock scheme, we are
only doing two things:

1. Putting all the duplicated code that handles write_urb_busy (that
   already exists) in only one place
2. Replacing a spin_lock that is being used to protect only a single
   integer field by an atomic_t

People got scared when they looked at the patch that does (1), thinking
that we were creating new, weird, locking scheme (because we are doing (2)
at the same time). But we are just isolating the existing 'write_urb_busy'
scheme that is duplicated all around the usb-serial drivers.

I guess (1) is a Good Thing, so the only question is: do you really
disagree about doing (2)?

-- 
Eduardo

Attachment: pgp00000.pgp
Description: PGP signature