Hi,
Background information:
compiled and tested a test11 with the Montavista preemptive patch.
After pressing Magic-SysRq-M all processes that tried to do IO hung in 'D'
Last message "Buffer memory ..."
Pressing Magic-SysRq-M again, all hung processes continued...
Checking the patch it looks like this
printk("Buffer memory: %6dkB\n",
atomic_read(&buffermem_pages) << (PAGE_SHIFT-10));
-#ifdef CONFIG_SMP /* trylock does nothing on UP and so we could deadlock */
- if (!spin_trylock(&lru_list_lock))
+#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT)
+ if (!mutex_trylock(&lru_list_mtx))
return;
for(nlist = 0; nlist < NR_LIST; nlist++) {
Ok, so I run some more code now than before (UP system with PREEMPT).
mutex_trylock is defined as:
+#define mutex_trylock(x) down_trylock(x)
Noticed that if the spin_trylock returns 0 on success, I will get the
behavior I see.
Not printing buffer info first time.
Holding the lock - stopping other fs processes.
Failing the mutex_trylock next attempt, interprete as success
- continuing and printing the buffer info.
- finally release the mutex
I removed the not (!) and now it works as expected.
Questions:
What are _trylocks supposed to return?
Does spin_trylock and down_trylock behave differently?
Why isn't the expected return value documented?
/RogerL
--
--
Home page:
http://www.norran.net/nra02596/
On Sat, 25 Nov 2000, Roger Larsson wrote:
> Questions:
> What are _trylocks supposed to return?
It depends on the type of _trylock ;(
> Does spin_trylock and down_trylock behave differently?
> Why isn't the expected return value documented?
The whole trylock stuff is, IMHO, a big mess. When you
change from one type of trylock to another, you may be
forced to invert the logic of your code since the return
code from the different locks is different.
For bitflags, for example, the trylock returns the state
the bit had before the lock (ie. 1 if the thing was already
locked).
For spinlocks, it'll probably return something else ;/
regards,
Rik
--
Hollywood goes for world dumbination,
Trailer at 11.
http://www.conectiva.com/ http://www.surriel.com/
On Sat, Nov 25, 2000 at 03:49:25PM -0200, Rik van Riel wrote:
> On Sat, 25 Nov 2000, Roger Larsson wrote:
>
> > Questions:
> > What are _trylocks supposed to return?
>
> It depends on the type of _trylock ;(
>
> > Does spin_trylock and down_trylock behave differently?
> > Why isn't the expected return value documented?
>
> The whole trylock stuff is, IMHO, a big mess. When you
> change from one type of trylock to another, you may be
> forced to invert the logic of your code since the return
> code from the different locks is different.
>
> For bitflags, for example, the trylock returns the state
> the bit had before the lock (ie. 1 if the thing was already
> locked).
I assume you're talking about test_and_{set,clear}_bit here. Their return
value isn't consistent with the other _trylock functions since they're not
_trylock functions.
I think the real problem is that people use test_and_set_bit for locks,
which is almost never[1] a good idea. The overhead for a semaphore shouldn't
be too much in most cases, and that way it is obvious what you want to do -
and, hopefully, even more obvious if you end up with a semaphore that can
be turned into a spinlock without further changes.
> For spinlocks, it'll probably return something else ;/
_trylock functions return 0 for success.
On Saturday 25 November 2000 18:49, Rik van Riel wrote:
> On Sat, 25 Nov 2000, Roger Larsson wrote:
> > Questions:
> > What are _trylocks supposed to return?
>
> It depends on the type of _trylock ;(
>
> > Does spin_trylock and down_trylock behave differently?
> > Why isn't the expected return value documented?
>
> The whole trylock stuff is, IMHO, a big mess. When you
> change from one type of trylock to another, you may be
> forced to invert the logic of your code since the return
> code from the different locks is different.
>
> For bitflags, for example, the trylock returns the state
> the bit had before the lock (ie. 1 if the thing was already
> locked).
>
This holds for down_trylocks too.
It looks like it is the spinlocks that are wrong... :-(
As most return values tend to be error returns that also
matches other code in functionallity.
>
> For spinlocks, it'll probably return something else ;/
It does...
I guess fixing this is too much too late?
It looks like ppc mixes the ways... from arch/ppc/lib/locks.c:46
int spin_trylock(spinlock_t *lock)
{
if (__spin_trylock(&lock->lock)) /* one on failure */
return 0; /* zero on failure */
lock->owner_cpu = smp_processor_id();
lock->owner_pc = (unsigned long)__builtin_return_address(0);
return 1;
}
BUT anyway...
The thing I hit is not a bug in the kernel proper - it is in the preemptive
stuff.
/RogerL
--
Home page:
http://www.norran.net/nra02596/
On Saturday 25 November 2000 19:30, Philipp Rumpf wrote:
> On Sat, Nov 25, 2000 at 03:49:25PM -0200, Rik van Riel wrote:
> > On Sat, 25 Nov 2000, Roger Larsson wrote:
> > > Questions:
> > > What are _trylocks supposed to return?
> >
> > It depends on the type of _trylock ;(
> >
> > > Does spin_trylock and down_trylock behave differently?
> > > Why isn't the expected return value documented?
> >
> > The whole trylock stuff is, IMHO, a big mess. When you
> > change from one type of trylock to another, you may be
> > forced to invert the logic of your code since the return
> > code from the different locks is different.
> >
> > For bitflags, for example, the trylock returns the state
> > the bit had before the lock (ie. 1 if the thing was already
> > locked).
>
> I assume you're talking about test_and_{set,clear}_bit here. Their return
> value isn't consistent with the other _trylock functions since they're not
> _trylock functions.
>
> I think the real problem is that people use test_and_set_bit for locks,
> which is almost never[1] a good idea. The overhead for a semaphore
> shouldn't be too much in most cases, and that way it is obvious what you
> want to do - and, hopefully, even more obvious if you end up with a
> semaphore that can be turned into a spinlock without further changes.
>
> > For spinlocks, it'll probably return something else ;/
>
> _trylock functions return 0 for success.
Not spin_trylock
Simple example code from
code from include/asm-mips/spinlock.h:65
#define spin_trylock(lock) (!test_and_set_bit(0,(lock)))
/RogerL
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> Please read the FAQ at http://www.tux.org/lkml/
--
--
Home page:
http://www.norran.net/nra02596/
On Sat, Nov 25, 2000 at 08:03:49PM +0100, Roger Larsson wrote:
> > _trylock functions return 0 for success.
>
> Not spin_trylock
Argh, I missed the (recent ?) change to make x86 spinlocks use 1 to mean
unlocked. You're correct, and obviously this should be fixed.
On Saturday 25 November 2000 20:22, Philipp Rumpf wrote:
> On Sat, Nov 25, 2000 at 08:03:49PM +0100, Roger Larsson wrote:
> > > _trylock functions return 0 for success.
> >
> > Not spin_trylock
>
> Argh, I missed the (recent ?) change to make x86 spinlocks use 1 to mean
> unlocked. You're correct, and obviously this should be fixed.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> Please read the FAQ at http://www.tux.org/lkml/
If this are to change in 2.4 I would suggest
to renaming it to mutex_lock (as in Nigels preemptive kernel patch)
Why?
A) the name spin_lock describes how the function is implemented and not
the intended purpose.
B) with a preemptive kernel we will have more than four intended purposes:
1) SMP - spin_lock, prevent two processors to run currently
2) UP - not used, code can only be executed by one thread.
3) PREEMTIVE - lock a region for preemption to avoid concurrent execution.
4) debug - addition of debug checks.
With Nigels patch most are changed, with some additional stuff...
My suggestion, change the name to mutex_lock and negate let mutex_trylock
follow the example of other _trylocks (returning 0 for success).
Ok?
If it is ok, I can prepare a patch (earliest monday)
/RogerL
--
Home page:
http://www.norran.net/nra02596/
On Saturday 25 November 2000 22:05, Roger Larsson wrote:
> On Saturday 25 November 2000 20:22, Philipp Rumpf wrote:
> > On Sat, Nov 25, 2000 at 08:03:49PM +0100, Roger Larsson wrote:
> > > > _trylock functions return 0 for success.
> > >
> > > Not spin_trylock
> >
> > Argh, I missed the (recent ?) change to make x86 spinlocks use 1 to mean
> > unlocked. You're correct, and obviously this should be fixed.
Have looked more into this now...
tasklet_trylock is also wrong (but there are only four of them)
Is this 2.4 only, or where there spin locks earlier too?
My suggestion now is a few steps:
1) to release a kernel version that has corrected _trylocks;
spin2_trylock and tasklet2_trylock.
[with corresponding updates in as many places as possible:
s/!spin_trylock/spin2_trylock/g
s/spin_trylock/!spin2_trylock/g
. . .]
(ready for spin trylock, not done for tasklet yet..., attached,
hope it got included OK - not fully used to kmail)
2) This will in house only drives or compilations that in some
strange way uses this calls...
3a) (DANGEROUS) global rename spin2_trylock to spin_trylock
[no logic change this time - only name]
3b) (dangerous) add compatibility interface
#define spin_trylock(L) (!spin2_trylock(L))
Probably not necessary since it can not be linked against.
Binary modules will contain their own compatibility code :-)
Probably preferred by those who maintain drivers for several
releases; 2.2, 2.4, ...
3c) do not do anything more...
Alternative:
1b) do nothing at all - suffer later
/RogerL
--
Home page:
http://www.norran.net/nra02596/
So what is a coder to do. We need to define the pi_mutex_trylock(). If
I understand this thread, it should return 0 on success. Is this
correct?
George
On Saturday 25 November 2000 22:05, Roger Larsson wrote:
> On Saturday 25 November 2000 20:22, Philipp Rumpf wrote:
> > On Sat, Nov 25, 2000 at 08:03:49PM +0100, Roger Larsson wrote:
> > > > _trylock functions return 0 for success.
> > >
> > > Not spin_trylock
> >
> > Argh, I missed the (recent ?) change to make x86 spinlocks use 1 to mean
> > unlocked. You're correct, and obviously this should be fixed.
Have looked more into this now...
tasklet_trylock is also wrong (but there are only four of them)
Is this 2.4 only, or where there spin locks earlier too?
My suggestion now is a few steps:
1) to release a kernel version that has corrected _trylocks;
spin2_trylock and tasklet2_trylock.
[with corresponding updates in as many places as possible:
s/!spin_trylock/spin2_trylock/g
s/spin_trylock/!spin2_trylock/g
. . .]
(ready for spin trylock, not done for tasklet yet..., attached,
hope it got included OK - not fully used to kmail)
2) This will in house only drives or compilations that in some
strange way uses this calls...
3a) (DANGEROUS) global rename spin2_trylock to spin_trylock
[no logic change this time - only name]
3b) (dangerous) add compatibility interface
#define spin_trylock(L) (!spin2_trylock(L))
Probably not necessary since it can not be linked against.
Binary modules will contain their own compatibility code :-)
Probably preferred by those who maintain drivers for several
releases; 2.2, 2.4, ...
3c) do not do anything more...
Alternative:
1b) do nothing at all - suffer later
/RogerL