Greg,
Don't get scared. :-)
As showed by Eduardo Habkost some days ago, the spin lock 'lock' in the
struct 'usb_serial_port' is being used by some USB serial drivers to protect
the access to the 'write_urb_busy' member of the same struct.
The spin lock however, is needless: we can change 'write_urb_busy' type
to be atomic_t and remove all the spin lock usage.
The following patch series does that. It introduces a very simple URB write
lock abstraction and four macros to do the same job currently done by the
spin lock.
The final result is a simpler and easy to read/understand code, with no
spin lock at all.
I've splited the work that way: the frist patch introduces the new macros;
from the second patch until the eight all the drivers are ported; patch
nine removes the 'lock' member from the usb-serial driver and patch ten
adds the write URB lock initialization for all the ports.
An important note is about the omninet driver. In its omninet_write_room()
method, it's accessing the 'write_urb_busy' member from the 'serial->port[1]'
port, and _not_ from the usb_serial_port passed as its argument. I have no
sure if it is right, but my port does perserve that semantic.
As I don't have any of the changed drivers, I have only made the compilation
test. Would be good to hold the patches in -mm for a while.
A final note: all the patches have been made with my usb-serial fixes
(which are already in your tree) applyed. They are:
usbserial-adds-missing-checks-and-bug-fix.patch
usbserial-race-condition-fix.patch
Thank you,
--
Luiz Fernando N. Capitulino
On Tue, Dec 06, 2005 at 09:56:10AM -0200, Luiz Fernando Capitulino wrote:
> Greg,
>
> Don't get scared. :-)
>
> As showed by Eduardo Habkost some days ago, the spin lock 'lock' in the
> struct 'usb_serial_port' is being used by some USB serial drivers to protect
> the access to the 'write_urb_busy' member of the same struct.
>
> The spin lock however, is needless: we can change 'write_urb_busy' type
> to be atomic_t and remove all the spin lock usage.
But if you do that, you make things slower on non-smp machines, which
isn't very nice. Why does the spinlock bother you?
thanks,
greg k-h
On Tue, Dec 06, 2005 at 11:40:41AM -0800, Greg KH wrote:
> On Tue, Dec 06, 2005 at 09:56:10AM -0200, Luiz Fernando Capitulino wrote:
> > Greg,
> >
> > Don't get scared. :-)
> >
> > As showed by Eduardo Habkost some days ago, the spin lock 'lock' in the
> > struct 'usb_serial_port' is being used by some USB serial drivers to protect
> > the access to the 'write_urb_busy' member of the same struct.
> >
> > The spin lock however, is needless: we can change 'write_urb_busy' type
> > to be atomic_t and remove all the spin lock usage.
>
> But if you do that, you make things slower on non-smp machines, which
> isn't very nice. Why does the spinlock bother you?
>
We thought that an atomic_t was better when you suggested that we could
drop the spinlock after we added a semaphore to struct usb_serial_port
recently. Won't we drop the spinlock as suggested?
Anyway, I don't see yet why the atomic_t would make the code slower on
non-smp. Is atomic_add_unless(v, 1, 1) supposed to be slower than
'if (!v) v = 1;' ?
If it is really slower, is atomic_cmpxchg() supposed to be slower, too?
("Check it yourself" is a valid answer, too :) But maybe someone
could elighten me and I could save some time testing and checking the
generated code)
--
Eduardo
On Tue, 6 Dec 2005 11:40:41 -0800
Greg KH <[email protected]> wrote:
| On Tue, Dec 06, 2005 at 09:56:10AM -0200, Luiz Fernando Capitulino wrote:
| > Greg,
| >
| > Don't get scared. :-)
| >
| > As showed by Eduardo Habkost some days ago, the spin lock 'lock' in the
| > struct 'usb_serial_port' is being used by some USB serial drivers to protect
| > the access to the 'write_urb_busy' member of the same struct.
| >
| > The spin lock however, is needless: we can change 'write_urb_busy' type
| > to be atomic_t and remove all the spin lock usage.
|
| But if you do that, you make things slower on non-smp machines, which
| isn't very nice. Why does the spinlock bother you?
The spinlock makes the code less clear, error prone, and we already a
semaphore in the struct usb_serial_port.
The spinlocks _seems_ useless to me.
--
Luiz Fernando N. Capitulino
On Tue, 6 Dec 2005 18:14:49 -0200, Luiz Fernando Capitulino <[email protected]> wrote:
> The spinlock makes the code less clear, error prone, and we already a
> semaphore in the struct usb_serial_port.
>
> The spinlocks _seems_ useless to me.
Dude, semaphores are not compatible with interrupts. Surely you
understand that?
-- Pete
Hi Pete,
On Tue, 6 Dec 2005 13:02:07 -0800
Pete Zaitcev <[email protected]> wrote:
| On Tue, 6 Dec 2005 18:14:49 -0200, Luiz Fernando Capitulino <[email protected]> wrote:
|
| > The spinlock makes the code less clear, error prone, and we already a
| > semaphore in the struct usb_serial_port.
| >
| > The spinlocks _seems_ useless to me.
|
| Dude, semaphores are not compatible with interrupts. Surely you
| understand that?
Sure thing man, take a look at this thread:
http://marc.theaimsgroup.com/?l=linux-kernel&m=113216151918308&w=2
My comment 'we already have a semaphore in struct usb_serial_port'
was about what we've discussed in that thread, where question like
'why should we have yet another lock here?' have been made.
And *not* 'let's use the semaphore instead'.
If _speed_ does not make difference, the spinlock seems useless,
because we could use atomic_t instead.
--
Luiz Fernando N. Capitulino
Am Dienstag, 6. Dezember 2005 22:18 schrieb Luiz Fernando Capitulino:
>
> Hi Pete,
>
> On Tue, 6 Dec 2005 13:02:07 -0800
> Pete Zaitcev <[email protected]> wrote:
>
> | On Tue, 6 Dec 2005 18:14:49 -0200, Luiz Fernando Capitulino <[email protected]> wrote:
> |
> | > The spinlock makes the code less clear, error prone, and we already a
> | > semaphore in the struct usb_serial_port.
> | >
> | > The spinlocks _seems_ useless to me.
> |
> | Dude, semaphores are not compatible with interrupts. Surely you
> | understand that?
>
> Sure thing man, take a look at this thread:
>
> http://marc.theaimsgroup.com/?l=linux-kernel&m=113216151918308&w=2
>
> My comment 'we already have a semaphore in struct usb_serial_port'
> was about what we've discussed in that thread, where question like
> 'why should we have yet another lock here?' have been made.
>
> And *not* 'let's use the semaphore instead'.
>
> If _speed_ does not make difference, the spinlock seems useless,
> because we could use atomic_t instead.
You can atomically set _one_ value using atomic_t. A spinlock allows
that and other more complex schemes.
Regards
Oliver
Am Dienstag, 6. Dezember 2005 21:13 schrieb Eduardo Pereira Habkost:
> Anyway, I don't see yet why the atomic_t would make the code slower on
> non-smp. Is atomic_add_unless(v, 1, 1) supposed to be slower than
> 'if (!v) v = 1;' ?
spin_lock() can be dropped on UP. atomic_XXX must either use an operation
on main memory, meaning less efficient code generation, or must disable
interrupts even on UP.
Regards
Oliver
On Tue, 6 Dec 2005 23:48:14 +0100
Oliver Neukum <[email protected]> wrote:
| Am Dienstag, 6. Dezember 2005 21:13 schrieb Eduardo Pereira Habkost:
| > Anyway, I don't see yet why the atomic_t would make the code slower on
| > non-smp. Is atomic_add_unless(v, 1, 1) supposed to be slower than
| > 'if (!v) v = 1;' ?
|
| spin_lock() can be dropped on UP. atomic_XXX must either use an operation
| on main memory, meaning less efficient code generation, or must disable
| interrupts even on UP.
Hmmm, I didn't know about the possibility to disable interrupts.
In the OOPS thread:
http://marc.theaimsgroup.com/?l=linux-usb-devel&m=113269682409774&w=2
*IIUC*, Greg told us that we could think about the possibility to drop
the spin lock and use the semaphore instead, because URB writes are slow.
We (me and Eduardo) didn't like it because we would be using the same
lock for two different problems, so we suggested the atomic_t, and Greg
agreed (IIRC).
Isn't it right? Is the URB write so fast that switching to atomic_t
doesn't pay-off?
--
Luiz Fernando N. Capitulino
On Tue, 6 Dec 2005 23:36:47 +0100
Oliver Neukum <[email protected]> wrote:
| Am Dienstag, 6. Dezember 2005 22:18 schrieb Luiz Fernando Capitulino:
| >
| > Hi Pete,
| >
| > On Tue, 6 Dec 2005 13:02:07 -0800
| > Pete Zaitcev <[email protected]> wrote:
| >
| > | On Tue, 6 Dec 2005 18:14:49 -0200, Luiz Fernando Capitulino <[email protected]> wrote:
| > |
| > | > The spinlock makes the code less clear, error prone, and we already a
| > | > semaphore in the struct usb_serial_port.
| > | >
| > | > The spinlocks _seems_ useless to me.
| > |
| > | Dude, semaphores are not compatible with interrupts. Surely you
| > | understand that?
| >
| > Sure thing man, take a look at this thread:
| >
| > http://marc.theaimsgroup.com/?l=linux-kernel&m=113216151918308&w=2
| >
| > My comment 'we already have a semaphore in struct usb_serial_port'
| > was about what we've discussed in that thread, where question like
| > 'why should we have yet another lock here?' have been made.
| >
| > And *not* 'let's use the semaphore instead'.
| >
| > If _speed_ does not make difference, the spinlock seems useless,
| > because we could use atomic_t instead.
|
| You can atomically set _one_ value using atomic_t. A spinlock allows
| that and other more complex schemes.
We only need to set 'write_urb_busy', nothing more.
--
Luiz Fernando N. Capitulino
> Isn't it right? Is the URB write so fast that switching to atomic_t
> doesn't pay-off?
an atomic_t access and a spinlock are basically the same price... so
what's the payoff ?
On Wed, 07 Dec 2005 13:27:13 +0100
Arjan van de Ven <[email protected]> wrote:
|
| > Isn't it right? Is the URB write so fast that switching to atomic_t
| > doesn't pay-off?
|
| an atomic_t access and a spinlock are basically the same price... so
| what's the payoff ?
One lock less, clean and less error prone code.
--
Luiz Fernando N. Capitulino
On Wed, 2005-12-07 at 10:30 -0200, Luiz Fernando Capitulino wrote:
> On Wed, 07 Dec 2005 13:27:13 +0100
> Arjan van de Ven <[email protected]> wrote:
>
> |
> | > Isn't it right? Is the URB write so fast that switching to atomic_t
> | > doesn't pay-off?
> |
> | an atomic_t access and a spinlock are basically the same price... so
> | what's the payoff ?
>
> One lock less,
where? spin_unlock in principle runs unlocked on x86 at least
(except for ppro workarounds but those are/should be optional)
On Wed, 07 Dec 2005 13:34:38 +0100
Arjan van de Ven <[email protected]> wrote:
| On Wed, 2005-12-07 at 10:30 -0200, Luiz Fernando Capitulino wrote:
| > On Wed, 07 Dec 2005 13:27:13 +0100
| > Arjan van de Ven <[email protected]> wrote:
| >
| > |
| > | > Isn't it right? Is the URB write so fast that switching to atomic_t
| > | > doesn't pay-off?
| > |
| > | an atomic_t access and a spinlock are basically the same price... so
| > | what's the payoff ?
| >
| > One lock less,
|
| where?
In the 'usb_serial_port', my patch number nine removes the spin lock.
| spin_unlock in principle runs unlocked on x86 at least
| (except for ppro workarounds but those are/should be optional)
|
|
--
Luiz Fernando N. Capitulino
On Wed, 7 Dec 2005 10:41:24 -0200
Luiz Fernando Capitulino <[email protected]> wrote:
| On Wed, 07 Dec 2005 13:34:38 +0100
| Arjan van de Ven <[email protected]> wrote:
|
| | On Wed, 2005-12-07 at 10:30 -0200, Luiz Fernando Capitulino wrote:
| | > On Wed, 07 Dec 2005 13:27:13 +0100
| | > Arjan van de Ven <[email protected]> wrote:
| | >
| | > |
| | > | > Isn't it right? Is the URB write so fast that switching to atomic_t
| | > | > doesn't pay-off?
| | > |
| | > | an atomic_t access and a spinlock are basically the same price... so
| | > | what's the payoff ?
| | >
| | > One lock less,
| |
| | where?
|
| In the 'usb_serial_port', my patch number nine removes the spin lock.
struct 'usb_serial_port' I meant.
--
Luiz Fernando N. Capitulino
Am Mittwoch, 7. Dezember 2005 13:25 schrieb Luiz Fernando Capitulino:
> On Tue, 6 Dec 2005 23:36:47 +0100
> Oliver Neukum <[email protected]> wrote:
>
> | Am Dienstag, 6. Dezember 2005 22:18 schrieb Luiz Fernando Capitulino:
> | >
> | > Hi Pete,
> | >
> | > On Tue, 6 Dec 2005 13:02:07 -0800
> | > Pete Zaitcev <[email protected]> wrote:
> | >
> | > | On Tue, 6 Dec 2005 18:14:49 -0200, Luiz Fernando Capitulino <[email protected]> wrote:
> | > |
> | > | > The spinlock makes the code less clear, error prone, and we already a
> | > | > semaphore in the struct usb_serial_port.
> | > | >
> | > | > The spinlocks _seems_ useless to me.
> | > |
> | > | Dude, semaphores are not compatible with interrupts. Surely you
> | > | understand that?
> | >
> | > Sure thing man, take a look at this thread:
> | >
> | > http://marc.theaimsgroup.com/?l=linux-kernel&m=113216151918308&w=2
> | >
> | > My comment 'we already have a semaphore in struct usb_serial_port'
> | > was about what we've discussed in that thread, where question like
> | > 'why should we have yet another lock here?' have been made.
> | >
> | > And *not* 'let's use the semaphore instead'.
> | >
> | > If _speed_ does not make difference, the spinlock seems useless,
> | > because we could use atomic_t instead.
> |
> | You can atomically set _one_ value using atomic_t. A spinlock allows
> | that and other more complex schemes.
>
> We only need to set 'write_urb_busy', nothing more.
So go hence and encapsulate that using the existent infrastructure. Thus
you get the most efficient solution.
Regards
Oliver
On Wed, 7 Dec 2005 14:01:25 +0100
Oliver Neukum <[email protected]> wrote:
| Am Mittwoch, 7. Dezember 2005 13:25 schrieb Luiz Fernando Capitulino:
| > On Tue, 6 Dec 2005 23:36:47 +0100
| > Oliver Neukum <[email protected]> wrote:
| >
| > | Am Dienstag, 6. Dezember 2005 22:18 schrieb Luiz Fernando Capitulino:
| > | >
| > | > Hi Pete,
| > | >
| > | > On Tue, 6 Dec 2005 13:02:07 -0800
| > | > Pete Zaitcev <[email protected]> wrote:
| > | >
| > | > | On Tue, 6 Dec 2005 18:14:49 -0200, Luiz Fernando Capitulino <[email protected]> wrote:
| > | > |
| > | > | > The spinlock makes the code less clear, error prone, and we already a
| > | > | > semaphore in the struct usb_serial_port.
| > | > | >
| > | > | > The spinlocks _seems_ useless to me.
| > | > |
| > | > | Dude, semaphores are not compatible with interrupts. Surely you
| > | > | understand that?
| > | >
| > | > Sure thing man, take a look at this thread:
| > | >
| > | > http://marc.theaimsgroup.com/?l=linux-kernel&m=113216151918308&w=2
| > | >
| > | > My comment 'we already have a semaphore in struct usb_serial_port'
| > | > was about what we've discussed in that thread, where question like
| > | > 'why should we have yet another lock here?' have been made.
| > | >
| > | > And *not* 'let's use the semaphore instead'.
| > | >
| > | > If _speed_ does not make difference, the spinlock seems useless,
| > | > because we could use atomic_t instead.
| > |
| > | You can atomically set _one_ value using atomic_t. A spinlock allows
| > | that and other more complex schemes.
| >
| > We only need to set 'write_urb_busy', nothing more.
|
| So go hence and encapsulate that using the existent infrastructure. Thus
| you get the most efficient solution.
Yes, I was speaking about it with Eduardo some minutes ago.
My only question is: currently the spin lock is not acquired for unlock
operations (ie, setting 'write_urb_busy' to 0), and to check
'write_usb_busy' value. I don't know if it's safe.
But, If I add the spin_lock()/spin_unlock() functions in my 'unlock'
and 'locked' methods, I could increase the latency for SMP systems.
Suggestions? Eduardo? Greg?
--
Luiz Fernando N. Capitulino
On Tue, 6 Dec 2005, Oliver Neukum wrote:
> Am Dienstag, 6. Dezember 2005 21:13 schrieb Eduardo Pereira Habkost:
> > Anyway, I don't see yet why the atomic_t would make the code slower on
> > non-smp. Is atomic_add_unless(v, 1, 1) supposed to be slower than
> > 'if (!v) v = 1;' ?
>
> spin_lock() can be dropped on UP. atomic_XXX must either use an operation
> on main memory, meaning less efficient code generation, or must disable
> interrupts even on UP.
atomic_add_unless is sort of a special case. It doesn't have a clean
simple implementation, unlike most of the other atomic_t operations. If
an equivalent result could be obtained using, e.g., atomic_inc_and_test,
that would be a better approach.
On the other hand, Oliver needs to be careful about claiming too much. In
general atomic_t operations _are_ superior to the spinlock approach. If
they weren't, atomic_t wouldn't belong in the kernel at all.
Alan Stern
> 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...
> 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>.
On Wed, 7 Dec 2005, Alan Stern wrote:
> On Tue, 6 Dec 2005, Oliver Neukum wrote:
>
>> Am Dienstag, 6. Dezember 2005 21:13 schrieb Eduardo Pereira Habkost:
>>> Anyway, I don't see yet why the atomic_t would make the code slower on
>>> non-smp. Is atomic_add_unless(v, 1, 1) supposed to be slower than
>>> 'if (!v) v = 1;' ?
>>
>> spin_lock() can be dropped on UP. atomic_XXX must either use an operation
>> on main memory, meaning less efficient code generation, or must disable
>> interrupts even on UP.
>
> atomic_add_unless is sort of a special case. It doesn't have a clean
> simple implementation, unlike most of the other atomic_t operations. If
> an equivalent result could be obtained using, e.g., atomic_inc_and_test,
> that would be a better approach.
>
> On the other hand, Oliver needs to be careful about claiming too much. In
> general atomic_t operations _are_ superior to the spinlock approach. If
> they weren't, atomic_t wouldn't belong in the kernel at all.
>
> Alan Stern
You need to know what it is that you intend to do if the code
encounters a locked section.
For example, let's pretend that every operation is atomic so
that only the logic is investigated...
if(!critical_section_flag) {
critical_section_flag = TRUE;
do_something_in_critical_section();
}
else
WTF?;
A spin-lock will prevent the current CPU from even getting to
or modifying data in the critical section because alternate paths
via interrupts are blocked. The only other way for data to be
modified is from another CPU. That CPU will spin until the current
CPU releases the lock.
Atomic operations on flags (semaphores) provide the opportunity
for another CPU to do something useful until the critical section
is released, the WTF above. However, if the other CPU can't
schedule you are caught between a rock and a hard-place because
you would need to spin anyway.
Basically, if you can schedule, it's much better to protect
a section with semaphores and do the down(&semi) / up(&semi) thing.
If you can't schedule, it's much cleaner to use a spin-lock
which, in fact, will prevent interference with the critical
section in most cases because, unless another CPU is idle,
it is unlikely to encounter the same thread of code.
Cheers,
Dick Johnson
Penguin : Linux version 2.6.13.4 on an i686 machine (5589.55 BogoMips).
Warning : 98.36% of all statistics are fiction.
.
****************************************************************
The information transmitted in this message is confidential and may be privileged. Any review, retransmission, dissemination, or other use of this information by persons or entities other than the intended recipient is prohibited. If you are not the intended recipient, please notify Analogic Corporation immediately - by replying to this message or by sending an email to [email protected] - and destroy all copies of this information, including any attachments, without reading or disclosing them.
Thank you.
Am Mittwoch, 7. Dezember 2005 16:22 schrieb Arjan van de Ven:
> > 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...
You are refering to SMP, aren't you?
Regards
Oliver
On Wed, 2005-12-07 at 16:37 +0100, Oliver Neukum wrote:
> Am Mittwoch, 7. Dezember 2005 16:22 schrieb Arjan van de Ven:
> > > 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...
>
> You are refering to SMP, aren't you?
yes.
on UP neither is a locked instruction ;)
Am Mittwoch, 7. Dezember 2005 16:40 schrieben Sie:
> On Wed, 2005-12-07 at 16:37 +0100, Oliver Neukum wrote:
> > Am Mittwoch, 7. Dezember 2005 16:22 schrieb Arjan van de Ven:
> > > > 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...
> >
> > You are refering to SMP, aren't you?
>
> yes.
> on UP neither is a locked instruction ;)
But the atomic variant has to guard against interrupts, at least on
architectures that do load/store only, hasn't it? AFAICT it is even
theoretically impossible to tell for the compiler whether a function
is always called with interrupts off.
Regards
Oliver
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
On Wed, 7 Dec 2005, 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...
You're overgeneralizing.
Sure, a locked cycle has a certain expense. But it's a lot less than the
expense of a contested spinlock. On the other hand, many times UP systems
can eliminate spinlocks entirely. There are lots of variables and many
possible tradeoffs.
Alan Stern
> > 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?
sounds like it...
> 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:
... however you are NOT changing the behavior, which is EXACTLY my
point; the current "lock emulation" behavior is wrong, all you're doing
is replacing how you do the wrong thing ;)
It's like having a bike with square wheels, and replacing a flat tire
with one with air in, as opposed to replacing it with a round wheel...
On Wed, 2005-12-07 at 11:01 -0500, Alan Stern wrote:
> On Wed, 7 Dec 2005, 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...
>
> You're overgeneralizing.
to some degree yes.
>
> Sure, a locked cycle has a certain expense. But it's a lot less than the
> expense of a contested spinlock.
the chances that *this* spinlock ends up being contested are near zero,
and.. in that scenario a locked cycle does the same thing, just in
hardware..... (eg the other cpu will busy wait until this locked cycle
is done)
On Mer, 2005-12-07 at 16:50 +0100, Oliver Neukum wrote:
> But the atomic variant has to guard against interrupts, at least on
> architectures that do load/store only, hasn't it?
Yes. And you will see at least four different approaches
1. ll/sc where if the sequence was interrupted and may be stale it gets
retried
2. locked operations where the IRQ cannot split the sequence and use of
3. spin locks to provide atomic operations where there are architecture
limits
4. Use of instructions acting on memory where the CPU in question has
them and (as is usual in processors) does not permit an IRQ mid
instruction.
Thus on x86
*foo++
might be atomic, might not on uniprocessor v interrupt solely because
the compiler chooses the operations. Atomic_inc however merely has to
use asm to force an inc of a memory location target. That instruction
cannot be split part way by an interrupt so is sufficient.
Relative efficiency of spin_lock versus atomic_foo is very platform
dependant.
On Wed, 7 Dec 2005, linux-os (Dick Johnson) wrote:
> You need to know what it is that you intend to do if the code
> encounters a locked section.
>
> For example, let's pretend that every operation is atomic so
> that only the logic is investigated...
>
> if(!critical_section_flag) {
> critical_section_flag = TRUE;
> do_something_in_critical_section();
> }
> else
> WTF?;
>
>
> A spin-lock will prevent the current CPU from even getting to
> or modifying data in the critical section because alternate paths
> via interrupts are blocked. The only other way for data to be
> modified is from another CPU. That CPU will spin until the current
> CPU releases the lock.
>
> Atomic operations on flags (semaphores) provide the opportunity
> for another CPU to do something useful until the critical section
> is released, the WTF above. However, if the other CPU can't
> schedule you are caught between a rock and a hard-place because
> you would need to spin anyway.
>
> Basically, if you can schedule, it's much better to protect
> a section with semaphores and do the down(&semi) / up(&semi) thing.
> If you can't schedule, it's much cleaner to use a spin-lock
> which, in fact, will prevent interference with the critical
> section in most cases because, unless another CPU is idle,
> it is unlikely to encounter the same thread of code.
That's true as far as it goes. But it ignores the possibility that, for
example, the critical section is extremely short (like incrementing an
integer variable). In such situations, spinning is better than
scheduling. And even better than spinning is for the CPU to wait while
another CPU carries out a locked bus cycle (which is what atomic_t
operations do on x86). As well as being more efficient, it may even be
"cleaner" -- depending on one's personal taste. :-)
Alan Stern
On Wed, Dec 07, 2005 at 05:02:33PM +0100, Arjan van de Ven wrote:
>
> > > 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?
>
> sounds like it...
>
> > 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:
>
> ... however you are NOT changing the behavior, which is EXACTLY my
> point; the current "lock emulation" behavior is wrong, all you're doing
> is replacing how you do the wrong thing ;)
But now doing the Right Thing will be easy, as the wrong code isn't
duplicated all around anymore: it is only in one place. ;)
We have just done a small refactoring, trying to keep behaviour.
I haven't analysed deeply the current code to check if the "lock
emulation" could be replaced by a better approach. But at a first look,
it didn't look wrong to me. I am open to suggestions on how to replace
the write_urb_busy checking by something better.
So, at least we agree that using atomic_t is better than the current
approach, right? So, do you agree that, _if_ we chose to keep the
write_urb_busy "pseudo-locking", we could at least remove the code
duplication for that and use an atomic_t instead of spin_lock+int?
>
> It's like having a bike with square wheels, and replacing a flat tire
> with one with air in, as opposed to replacing it with a round wheel...
>
I am open to suggestions on how to build a round wheel in this case. :)
--
Eduardo
On Tue, Dec 06, 2005 at 09:56:10AM -0200, Luiz Fernando Capitulino wrote:
> Greg,
>
> Don't get scared. :-)
I'm not scared, just not liking this patch series at all.
In the end, it's just moving from one locking scheme to another. No big
deal.
The problem is, none of this should be needed at all. We need to move
the usb-serial drivers over to use the serial core code. If that
happens, then none of this locking is needed.
That's the right thing to do, so I'm not going to take this patch series
right now because of that. If you all want to work on moving to use the
serial core, I would love to see that happen.
thanks,
greg k-h
On Wed, 7 Dec 2005 08:41:18 -0800
Greg KH <[email protected]> wrote:
| On Tue, Dec 06, 2005 at 09:56:10AM -0200, Luiz Fernando Capitulino wrote:
| > Greg,
| >
| > Don't get scared. :-)
|
| I'm not scared, just not liking this patch series at all.
|
| In the end, it's just moving from one locking scheme to another. No big
| deal.
I understand.
| The problem is, none of this should be needed at all. We need to move
| the usb-serial drivers over to use the serial core code. If that
| happens, then none of this locking is needed.
|
| That's the right thing to do, so I'm not going to take this patch series
| right now because of that. If you all want to work on moving to use the
| serial core, I would love to see that happen.
If it's the right thing to do, I'll love to work on that. :)
There is only one problem though, I've never touched in the serial core.
It means I'll need some time to do it, and maybe the first tries can be
wrong.
Any tips you have in mind are very welcome.
Eduardo, let's do it? :)
--
Luiz Fernando N. Capitulino
Greg KH <[email protected]> writes:
> That's the right thing to do, so I'm not going to take this patch series
> right now because of that. If you all want to work on moving to use the
> serial core, I would love to see that happen.
But wouldn't be better to have this intermediary solution merged while
someone work on this conversion?
--
O T A V I O S A L V A D O R
---------------------------------------------
E-mail: [email protected] UIN: 5906116
GNU/Linux User: 239058 GPG ID: 49A5F855
Home Page: http://www.freedom.ind.br/otavio
---------------------------------------------
"Microsoft gives you Windows ... Linux gives
you the whole house."
On Wed, Dec 07, 2005 at 02:55:07PM -0200, Otavio Salvador wrote:
> Greg KH <[email protected]> writes:
>
> > That's the right thing to do, so I'm not going to take this patch series
> > right now because of that. If you all want to work on moving to use the
> > serial core, I would love to see that happen.
>
> But wouldn't be better to have this intermediary solution merged while
> someone work on this conversion?
No, why do you say that? It doesn't fix a bug at all, and it isn't a
"solution" to any existing problem.
thanks,
greg k-h
On Wed, Dec 07, 2005 at 02:51:13PM -0200, Luiz Fernando Capitulino wrote:
> On Wed, 7 Dec 2005 08:41:18 -0800
> Greg KH <[email protected]> wrote:
>
> | On Tue, Dec 06, 2005 at 09:56:10AM -0200, Luiz Fernando Capitulino wrote:
> | > Greg,
> | >
> | > Don't get scared. :-)
> |
> | I'm not scared, just not liking this patch series at all.
> |
> | In the end, it's just moving from one locking scheme to another. No big
> | deal.
>
> I understand.
>
> | The problem is, none of this should be needed at all. We need to move
> | the usb-serial drivers over to use the serial core code. If that
> | happens, then none of this locking is needed.
> |
> | That's the right thing to do, so I'm not going to take this patch series
> | right now because of that. If you all want to work on moving to use the
> | serial core, I would love to see that happen.
>
> If it's the right thing to do, I'll love to work on that. :)
>
> There is only one problem though, I've never touched in the serial core.
> It means I'll need some time to do it, and maybe the first tries can be
> wrong.
>
> Any tips you have in mind are very welcome.
I have a small question: in my view, this patch series is a small
step towards implementing the usb-serial drivers The Right Way, as it
removes a a bit of duplicated code. If we start to do The Big Change to
serial_core , probably we would make further refactorings on these parts,
going towards The Right Way to implement the drivers.
My question would be: where would the small refactorings belong, while
the big change to serial_core is work in progress? I would like them
to go to some tree for testing, while the work is being done, instead
of pushing lots of changes later, but I don't know if there is someone
who we could send them.
>
> Eduardo, let's do it? :)
I would love it, but I will be on vacations in two weeks. So, probably
on January.
My wife is lucky that I won't have a notebook available during our
vacations. 8)
--
Eduardo
On Wed, Dec 07, 2005 at 03:13:32PM -0200, Eduardo Pereira Habkost wrote:
> I have a small question: in my view, this patch series is a small
> step towards implementing the usb-serial drivers The Right Way, as it
> removes a a bit of duplicated code.
It doesn't remove any "duplicated code", it only changes a spinlock to
an atomic_t for one single value (which I personally do not think is the
best thing to do, and based on the number of comments on this thread, I
think others also feel this way.)
> If we start to do The Big Change to serial_core , probably we would
> make further refactorings on these parts, going towards The Right Way
> to implement the drivers.
Sure, that's the way kernel development is done.
> My question would be: where would the small refactorings belong, while
> the big change to serial_core is work in progress? I would like them
> to go to some tree for testing, while the work is being done, instead
> of pushing lots of changes later, but I don't know if there is someone
> who we could send them.
The "normal" way of doing work like this is, do it somewhere, and then
break it all down into a series of steps, after you have figured out
exactly where you are going to end up.
Feel free to send me any patches that you feel should be applied that
work toward this end goal.
thanks,
greg k-h
Hi, Greg,
On Wed, Dec 07, 2005 at 09:56:14AM -0800, Greg KH wrote:
> On Wed, Dec 07, 2005 at 03:13:32PM -0200, Eduardo Pereira Habkost wrote:
> > I have a small question: in my view, this patch series is a small
> > step towards implementing the usb-serial drivers The Right Way, as it
> > removes a a bit of duplicated code.
>
> It doesn't remove any "duplicated code", it only changes a spinlock to
> an atomic_t for one single value (which I personally do not think is the
> best thing to do, and based on the number of comments on this thread, I
> think others also feel this way.)
Every usb-serial driver currently has:
spin_lock(port->lock);
if (port->write_urb_busy)
return;
port->write_urb_busy = 1;
spin_unlock(port->lock);
Having the same 5 lines on many usb-serial drivers looks like duplicated
code to me. Maybe I am being too exigent. Anyway, this is unrelated to
the atomic_t change, and we could do it without dropping ths spinlock.
But I would like to know: would you would accept such change (that
encapsulate this write_urb_busy logic without necessarily dropping the
spinlock)?
And, about the atomic_t: I've felt most people didn't liked the atomic_t
approach for one of two reasons:
1. The existence of write_urb_busy looks like a hack, and we've
make it explicit when we isolated the code that uses write_urb_busy
in a set of functions (the point of Arjan in his "square wheel"
comment)
2. The whole discussion about atomic_t vs. spinlock efficiency
I agree with (1), but I still don't see why using a spinlock to protect
a single field is better than using atomic_t. Both in code readability
and efficiency. Specially as the difference between each one (even which
one is more efficient) is very arch-specific.
Thank you very much for your advice,
--
Eduardo