* [PATCH 2.6.12.5 1/2] lib: allow idr to be used in irq context
@ 2005-08-16 22:03 Luben Tuikov
2005-08-17 16:01 ` Jim Houston
0 siblings, 1 reply; 18+ messages in thread
From: Luben Tuikov @ 2005-08-16 22:03 UTC (permalink / raw)
To: Linux Kernel Mailing List, SCSI Mailing List, Dave Jones,
Jeff Garzik
Cc: Jim Houston
Hi,
This patch allows idr to be used in irq context.
idr_pre_get() is necessary to be called before
idr_get_new() is called. No locking is necessary when
calling idr_pre_get().
But idr_get_new(), idr_find() and idr_remove()
must be serialized with respect to each other.
All of the aforementioned, may end up in alloc_layer()
or free_layer() which grabs the idp lock using spin_lock.
If idr_get_new() or idr_remove() is used in IRQ context,
then we may get a lockup when idr_pre_get was called
in process context and an IRQ interrupted while it held
the idp lock.
This patch changes the spin_lock to spin_lock_irqsave,
and spin_unlock to spin_unlock_irqrestore, to allow
idr_get_new(), idr_find() and idr_remove() to be
called from IRQ context, while idr_pre_get() to be
called in process context.
Signed-off-by: Luben Tuikov <luben_tuikov@adaptec.com>
--- linux-2.6.12.5/lib/idr.c.old 2005-08-16 17:20:08.000000000 -0400
+++ linux-2.6.12.5/lib/idr.c 2005-08-16 17:22:16.000000000 -0400
@@ -37,27 +37,29 @@
static struct idr_layer *alloc_layer(struct idr *idp)
{
struct idr_layer *p;
+ unsigned long flags;
- spin_lock(&idp->lock);
+ spin_lock_irqsave(&idp->lock, flags);
if ((p = idp->id_free)) {
idp->id_free = p->ary[0];
idp->id_free_cnt--;
p->ary[0] = NULL;
}
- spin_unlock(&idp->lock);
+ spin_unlock_irqrestore(&idp->lock, flags);
return(p);
}
static void free_layer(struct idr *idp, struct idr_layer *p)
{
+ unsigned long flags;
/*
* Depends on the return element being zeroed.
*/
- spin_lock(&idp->lock);
+ spin_lock_irqsave(&idp->lock, flags);
p->ary[0] = idp->id_free;
idp->id_free = p;
idp->id_free_cnt++;
- spin_unlock(&idp->lock);
+ spin_unlock_irqrestore(&idp->lock, flags);
}
/**
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.6.12.5 1/2] lib: allow idr to be used in irq context
2005-08-16 22:03 [PATCH 2.6.12.5 1/2] lib: allow idr to be used in irq context Luben Tuikov
@ 2005-08-17 16:01 ` Jim Houston
2005-08-21 8:25 ` Andrew Morton
0 siblings, 1 reply; 18+ messages in thread
From: Jim Houston @ 2005-08-17 16:01 UTC (permalink / raw)
To: Luben Tuikov
Cc: Linux Kernel Mailing List, SCSI Mailing List, Dave Jones,
Jeff Garzik
On Tue, 2005-08-16 at 18:03, Luben Tuikov wrote:
> If idr_get_new() or idr_remove() is used in IRQ context,
> then we may get a lockup when idr_pre_get was called
> in process context and an IRQ interrupted while it held
> the idp lock.
Hi Everyone,
Luben's changes make sense please merge them.
Jim Houston - Concurrent Computer Corp.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.6.12.5 1/2] lib: allow idr to be used in irq context
2005-08-17 16:01 ` Jim Houston
@ 2005-08-21 8:25 ` Andrew Morton
2005-08-21 15:49 ` Luben Tuikov
0 siblings, 1 reply; 18+ messages in thread
From: Andrew Morton @ 2005-08-21 8:25 UTC (permalink / raw)
To: Jim Houston; +Cc: luben_tuikov, linux-kernel, linux-scsi, davej, jgarzik
Jim Houston <jim.houston@ccur.com> wrote:
>
> On Tue, 2005-08-16 at 18:03, Luben Tuikov wrote:
>
> > If idr_get_new() or idr_remove() is used in IRQ context,
> > then we may get a lockup when idr_pre_get was called
> > in process context and an IRQ interrupted while it held
> > the idp lock.
>
> Hi Everyone,
>
> Luben's changes make sense please merge them.
>
Well yes, the change makes sense if there's actually a caller which needs it.
If there is such a caller then Luben should identify it, please.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.6.12.5 1/2] lib: allow idr to be used in irq context
2005-08-21 8:25 ` Andrew Morton
@ 2005-08-21 15:49 ` Luben Tuikov
2005-08-21 16:06 ` James Bottomley
0 siblings, 1 reply; 18+ messages in thread
From: Luben Tuikov @ 2005-08-21 15:49 UTC (permalink / raw)
To: Andrew Morton, Jim Houston
Cc: luben_tuikov, linux-kernel, linux-scsi, davej, jgarzik
--- Andrew Morton <akpm@osdl.org> wrote:
> Jim Houston <jim.houston@ccur.com> wrote:
> >
> > On Tue, 2005-08-16 at 18:03, Luben Tuikov wrote:
> >
> > > If idr_get_new() or idr_remove() is used in IRQ context,
> > > then we may get a lockup when idr_pre_get was called
> > > in process context and an IRQ interrupted while it held
> > > the idp lock.
> >
> > Hi Everyone,
> >
> > Luben's changes make sense please merge them.
> >
>
> Well yes, the change makes sense if there's actually a caller which needs it.
>
> If there is such a caller then Luben should identify it, please.
Hi Andrew,
The caller is the aic94xx SAS LLDD. It uses IDR to generate unique
task tag for each SCSI task being submitted. It is then used to lookup
the task given the task tag, in effect using IDR as a fast lookup table.
Yes, I'm also not aware of any other users of IDR from mixed process/IRQ
context or for SCSI Task tag purposes.
Luben
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.6.12.5 1/2] lib: allow idr to be used in irq context
2005-08-21 15:49 ` Luben Tuikov
@ 2005-08-21 16:06 ` James Bottomley
2005-08-21 17:27 ` Luben Tuikov
2005-08-22 14:06 ` Luben Tuikov
0 siblings, 2 replies; 18+ messages in thread
From: James Bottomley @ 2005-08-21 16:06 UTC (permalink / raw)
To: luben_tuikov
Cc: Andrew Morton, Jim Houston, Linux Kernel, SCSI Mailing List,
Dave Jones, Jeff Garzik
On Sun, 2005-08-21 at 08:49 -0700, Luben Tuikov wrote:
> The caller is the aic94xx SAS LLDD. It uses IDR to generate unique
> task tag for each SCSI task being submitted. It is then used to lookup
> the task given the task tag, in effect using IDR as a fast lookup table.
>
> Yes, I'm also not aware of any other users of IDR from mixed process/IRQ
> context or for SCSI Task tag purposes.
Just a minute, that's not what idr was designed for. It was really
designed for enumerations (like disk) presented to the user. That's why
using it in IRQ context hasn't been considered.
However, there is an infrastructure in the block layer called the
generic tag infrastructure which was designed precisely for this purpose
and which is designed to operate in IRQ context.
James
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.6.12.5 1/2] lib: allow idr to be used in irq context
2005-08-21 16:06 ` James Bottomley
@ 2005-08-21 17:27 ` Luben Tuikov
2005-08-21 22:03 ` James Bottomley
2005-08-22 14:06 ` Luben Tuikov
1 sibling, 1 reply; 18+ messages in thread
From: Luben Tuikov @ 2005-08-21 17:27 UTC (permalink / raw)
To: James Bottomley, luben_tuikov
Cc: Andrew Morton, Jim Houston, Linux Kernel, SCSI Mailing List,
Dave Jones, Jeff Garzik
--- James Bottomley <James.Bottomley@SteelEye.com> wrote:
> On Sun, 2005-08-21 at 08:49 -0700, Luben Tuikov wrote:
> > The caller is the aic94xx SAS LLDD. It uses IDR to generate unique
> > task tag for each SCSI task being submitted. It is then used to lookup
> > the task given the task tag, in effect using IDR as a fast lookup table.
> >
> > Yes, I'm also not aware of any other users of IDR from mixed process/IRQ
> > context or for SCSI Task tag purposes.
>
> Just a minute, that's not what idr was designed for. It was really
> designed for enumerations (like disk) presented to the user. That's why
> using it in IRQ context hasn't been considered.
Hi James, how are you?
Is this the only use _you_ could find for a *radix tree*? ;-)
Since of course sd.c uses it just as an enumeration, according to
you this must be the only use? :-)
It was designed as a general purpose id to pointer translation
service, just as the comment in it says.
> However, there is an infrastructure in the block layer called the
> generic tag infrastructure which was designed precisely for this purpose
> and which is designed to operate in IRQ context.
James, I'm sure you're well aware that,
- a request_queue is LU-bound,
- a SCSI _transport_ (*ANY*) can _only_ address domain devices, but
_not_ LUs. LUs are *not* seen on the domain.
See the different associations? Then why are you posting such emails?
Andrew, please apply this patch.
Thanks,
Luben
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.6.12.5 1/2] lib: allow idr to be used in irq context
@ 2005-08-21 20:40 Luben Tuikov
0 siblings, 0 replies; 18+ messages in thread
From: Luben Tuikov @ 2005-08-21 20:40 UTC (permalink / raw)
To: James Bottomley, luben_tuikov
Cc: Andrew Morton, Jim Houston, Linux Kernel, SCSI Mailing List,
Dave Jones, Jeff Garzik
--- Luben Tuikov <luben_tuikov@adaptec.com> wrote:
> --- James Bottomley <James.Bottomley@SteelEye.com> wrote:
> > However, there is an infrastructure in the block layer called the
> > generic tag infrastructure which was designed precisely for this purpose
> > and which is designed to operate in IRQ context.
>
> James, I'm sure you're well aware that,
> - a request_queue is LU-bound,
> - a SCSI _transport_ (*ANY*) can _only_ address domain devices, but
> _not_ LUs. LUs are *not* seen on the domain.
Another thing very important to mention is that the layering
infrastructure should _not_ be broken, whereby LLDD are required
to use block layer tags.
First, there may not be one to one correspondence accross layers.
Second, a tag on a layer identifies that particular _layer_ object.
It should not be used across layers or across several layers:
Block->Scsi Core->LLDD.
Third, although SCSI task tags are a SAM concept, they are defined
by the protocol in use and generated and assigned by the Initiator
port. The block layer is _not_ an Initiator port. SCSI Core is
also _not_ an Initiator port.
SCSI Core should be impervious to task tags, i.e. to the Q in
I_T_L_Q nexus. The Q is never visible to it.
Luben
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.6.12.5 1/2] lib: allow idr to be used in irq context
2005-08-21 17:27 ` Luben Tuikov
@ 2005-08-21 22:03 ` James Bottomley
2005-08-22 0:33 ` Luben Tuikov
0 siblings, 1 reply; 18+ messages in thread
From: James Bottomley @ 2005-08-21 22:03 UTC (permalink / raw)
To: luben_tuikov
Cc: Andrew Morton, Jim Houston, Linux Kernel, SCSI Mailing List,
Dave Jones, Jeff Garzik
On Sun, 2005-08-21 at 10:27 -0700, Luben Tuikov wrote:
> Is this the only use _you_ could find for a *radix tree*? ;-)
> Since of course sd.c uses it just as an enumeration, according to
> you this must be the only use? :-)
And Dicto Simpliciter to you too.
> It was designed as a general purpose id to pointer translation
> service, just as the comment in it says.
idr was specifically designed with unlocked pre-allocation in mind. The
change you're proposing wants to allow pre allocation from irq context.
This really doesn't look like a good change because the whole point of
pre allocation is to do it at a point in time when the system can sleep
if it has to.
What I don't understand is why you need to pre allocate this tag from
irq context. Best practise is to allocate when you have user context
but make use of it in IRQ context.
> > However, there is an infrastructure in the block layer called the
> > generic tag infrastructure which was designed precisely for this purpose
> > and which is designed to operate in IRQ context.
>
> James, I'm sure you're well aware that,
> - a request_queue is LU-bound,
> - a SCSI _transport_ (*ANY*) can _only_ address domain devices, but
> _not_ LUs. LUs are *not* seen on the domain.
>
> See the different associations? Then why are you posting such emails?
So you're planning to do some type of tag command queueing outside of
the block framework? Could you just look at what it would take to do it
within the existing framework first? I seem to have wasted quite a bit
of time recently pulling spurious queueing code out of Adaptec drivers.
Remember that the code is adaptable ... we have non-block devices that
use block queues for instance.
James
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.6.12.5 1/2] lib: allow idr to be used in irq context
2005-08-21 22:03 ` James Bottomley
@ 2005-08-22 0:33 ` Luben Tuikov
2005-08-22 3:15 ` James Bottomley
0 siblings, 1 reply; 18+ messages in thread
From: Luben Tuikov @ 2005-08-22 0:33 UTC (permalink / raw)
To: James Bottomley, luben_tuikov
Cc: Andrew Morton, Jim Houston, Linux Kernel, SCSI Mailing List,
Dave Jones, Jeff Garzik
--- James Bottomley <James.Bottomley@SteelEye.com> wrote:
> On Sun, 2005-08-21 at 10:27 -0700, Luben Tuikov wrote:
> > Is this the only use _you_ could find for a *radix tree*? ;-)
> > Since of course sd.c uses it just as an enumeration, according to
> > you this must be the only use? :-)
>
> And Dicto Simpliciter to you too.
>
> > It was designed as a general purpose id to pointer translation
> > service, just as the comment in it says.
>
> idr was specifically designed with unlocked pre-allocation in mind. The
> change you're proposing wants to allow pre allocation from irq context.
> This really doesn't look like a good change because the whole point of
> pre allocation is to do it at a point in time when the system can sleep
> if it has to.
No preallocation is done from IRQ context. Do not spread FUD.
It seems to me that you're unaware how IDR works and unaware how
the driver works.
See the original patch submission -- I do explain why it is needed,
and show exactly how it breaks. You may also want to look at the idr.c
code. There is no reason not to promote the lock to an irq spinlock,
again, since it protects only data manipulation, as it should.
In fact, when you start writing code and actually face problems,
and start looking for the right tools, then you'll see it. Right now
this is a useless argument and you should drop it.
> What I don't understand is why you need to pre allocate this tag from
> irq context. Best practise is to allocate when you have user context
> but make use of it in IRQ context.
No preallocation is done in IRQ context. See above.
Furthermore, you should know that a slab cache *settles* very
quickly, especially for IO. You should've learned this when
I posted my results of SCSI Core using the slab cache for commands
5 years ago, before that code was in SCSI Core.
James, is this the most important thing for SCSI Core for the maintaner of
SCSI Core to discuss?
IDR's use? A general library utility's use in a driver?
Is this what the SCSI Core maintainer will use to reject a driver's
acceptance?
When so many things have been said in the last week on this mailing
list, being in direct importance to SCSI Core: error handling
being one of them, another is HCIL usage, and other vendor
provided labels for LUs, native target support, etc.
> > > However, there is an infrastructure in the block layer called the
> > > generic tag infrastructure which was designed precisely for this purpose
> > > and which is designed to operate in IRQ context.
> >
> > James, I'm sure you're well aware that,
> > - a request_queue is LU-bound,
> > - a SCSI _transport_ (*ANY*) can _only_ address domain devices, but
> > _not_ LUs. LUs are *not* seen on the domain.
> >
> > See the different associations? Then why are you posting such emails?
>
> So you're planning to do some type of tag command queueing outside of
> the block framework? Could you just look at what it would take to do it
> within the existing framework first? I seem to have wasted quite a bit
> of time recently pulling spurious queueing code out of Adaptec drivers.
No one is implementing tag command queueing. Why are you spreading FUD
into making people think that if you use tags, you must be doing
tagged command _queueing_?
Here:
Tags do _not_ imply TCQ.
TCQ does imply Tags.
> Remember that the code is adaptable ... we have non-block devices that
> use block queues for instance.
Completely irrelevant for a transport driver. Stop throwing red-herring.
Instead let's start discussing matters SCSI, which have been due for
the last 5 years, which I had hoped you'd _lead_ or at least
*listen and take paches* into SCSI Core, you know, like other
maintainers do. Those are:
- 8 byte LUNs, opaque to SCSI Core,
- native target (domain device) support,
- native LU discovery,
- HCIL becoming just one of _many_ possible labels for an LU, whereby
vendors provide them.
*This* is what is important and relevant to SCSI Core. Not to mention,
error handling, etc.
Now let's drop this and start discussing things which have been
a pain for the last 5 years, since the first SAM LLDD (iSCSI)
was introduced to SCSI Core (at least not publicly).
Let's take SCSI Core into the 21st century, shall we? Please?
Luben
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.6.12.5 1/2] lib: allow idr to be used in irq context
2005-08-22 0:33 ` Luben Tuikov
@ 2005-08-22 3:15 ` James Bottomley
2005-08-22 3:52 ` Andrew Morton
0 siblings, 1 reply; 18+ messages in thread
From: James Bottomley @ 2005-08-22 3:15 UTC (permalink / raw)
To: luben_tuikov
Cc: Andrew Morton, Jim Houston, Linux Kernel, SCSI Mailing List,
Dave Jones, Jeff Garzik
On Sun, 2005-08-21 at 17:33 -0700, Luben Tuikov wrote:
> No preallocation is done from IRQ context. Do not spread FUD.
> It seems to me that you're unaware how IDR works and unaware how
> the driver works.
Argumentum ad Hominem now ... we'll get them all eventually.
Since you won't post the usage code, just answer this: how does what
you're doing with idr differ from its originally designed consumer: the
posix timers which also do the idr_remove() in IRQ context?
To me it seems your argument applies to this case as well, but you
didn't quote it when asked for an example, so I assume there's some
difference that I don't understand.
James
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.6.12.5 1/2] lib: allow idr to be used in irq context
2005-08-22 3:15 ` James Bottomley
@ 2005-08-22 3:52 ` Andrew Morton
2005-08-22 14:28 ` James Bottomley
2005-08-22 16:33 ` Luben Tuikov
0 siblings, 2 replies; 18+ messages in thread
From: Andrew Morton @ 2005-08-22 3:52 UTC (permalink / raw)
To: James Bottomley
Cc: luben_tuikov, jim.houston, linux-kernel, linux-scsi, davej,
jgarzik
James Bottomley <James.Bottomley@SteelEye.com> wrote:
>
> Since you won't post the usage code, just answer this: how does what
> you're doing with idr differ from its originally designed consumer: the
> posix timers which also do the idr_remove() in IRQ context?
erp. posix_timers has its own irq-safe lock, so we're doing extra,
unneeded locking in that code path.
I think providing locking inside idr.c was always a mistake - generally we
rely on caller-provided locking for such things.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.6.12.5 1/2] lib: allow idr to be used in irq context
2005-08-21 16:06 ` James Bottomley
2005-08-21 17:27 ` Luben Tuikov
@ 2005-08-22 14:06 ` Luben Tuikov
1 sibling, 0 replies; 18+ messages in thread
From: Luben Tuikov @ 2005-08-22 14:06 UTC (permalink / raw)
To: James Bottomley, luben_tuikov
Cc: Andrew Morton, Jim Houston, Linux Kernel, SCSI Mailing List,
Dave Jones, Jeff Garzik
--- James Bottomley <James.Bottomley@SteelEye.com> wrote:
> On Sun, 2005-08-21 at 08:49 -0700, Luben Tuikov wrote:
> > The caller is the aic94xx SAS LLDD. It uses IDR to generate unique
> > task tag for each SCSI task being submitted. It is then used to lookup
> > the task given the task tag, in effect using IDR as a fast lookup table.
> >
> > Yes, I'm also not aware of any other users of IDR from mixed process/IRQ
> > context or for SCSI Task tag purposes.
>
> Just a minute, that's not what idr was designed for. It was really
> designed for enumerations (like disk) presented to the user. That's why
> using it in IRQ context hasn't been considered.
Hi James, how are you?
Is this the only use _you_ could find for a *radix tree*? ;-)
Since of course sd.c uses it just as an enumeration, according to
you this must be the only use? :-)
It was designed as a general purpose id to pointer translation
service, just as the comment in it says.
> However, there is an infrastructure in the block layer called the
> generic tag infrastructure which was designed precisely for this purpose
> and which is designed to operate in IRQ context.
James, I'm sure you're well aware that,
- a request_queue is LU-bound,
- a SCSI _transport_ (*ANY*) can _only_ address domain devices, but
_not_ LUs. LUs are *not* seen on the domain.
See the different associations? Then why are you posting such emails?
Andrew, please apply this patch.
Thanks,
Luben
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.6.12.5 1/2] lib: allow idr to be used in irq context
2005-08-22 3:52 ` Andrew Morton
@ 2005-08-22 14:28 ` James Bottomley
2005-08-22 16:51 ` Luben Tuikov
2005-08-22 21:53 ` James Bottomley
2005-08-22 16:33 ` Luben Tuikov
1 sibling, 2 replies; 18+ messages in thread
From: James Bottomley @ 2005-08-22 14:28 UTC (permalink / raw)
To: Andrew Morton
Cc: luben_tuikov, jim.houston, Linux Kernel, SCSI Mailing List,
Dave Jones, Jeff Garzik
On Sun, 2005-08-21 at 20:52 -0700, Andrew Morton wrote:
> erp. posix_timers has its own irq-safe lock, so we're doing extra,
> unneeded locking in that code path.
Possibly, the posix timer code is rather convoluted in this area so I'm
not entirely sure my analysis is correct.
> I think providing locking inside idr.c was always a mistake - generally we
> rely on caller-provided locking for such things.
Well, the reason is because they wanted lockless pre-alloc. If you do
it locked, you can't use GFP_KERNEL for the memory allocation flag which
rather defeats its purpose.
Perhaps the bug is in the API. We have pre-allocate, new, find and
remove. Perhaps what we're missing is a reuse all of which could then
rely on caller provided locking, with pre-alloc and remove requiring
user context but new, find and reuse being happy in irq context.
James
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.6.12.5 1/2] lib: allow idr to be used in irq context
2005-08-22 3:52 ` Andrew Morton
2005-08-22 14:28 ` James Bottomley
@ 2005-08-22 16:33 ` Luben Tuikov
1 sibling, 0 replies; 18+ messages in thread
From: Luben Tuikov @ 2005-08-22 16:33 UTC (permalink / raw)
To: Andrew Morton
Cc: James Bottomley, jim.houston, linux-kernel, linux-scsi, davej,
jgarzik
On 08/21/05 23:52, Andrew Morton wrote:
> James Bottomley <James.Bottomley@SteelEye.com> wrote:
>
>>Since you won't post the usage code, just answer this: how does what
>> you're doing with idr differ from its originally designed consumer: the
>> posix timers which also do the idr_remove() in IRQ context?
>
>
> erp. posix_timers has its own irq-safe lock, so we're doing extra,
> unneeded locking in that code path.
>
> I think providing locking inside idr.c was always a mistake - generally we
> rely on caller-provided locking for such things.
Ahhh, *THANK YOU* Andrew for your common sense!
Yes, James is unaware that 3 out of the 4 major entrances into IDR
_must_ be synchronized with respect to each other, depending
on your context (irq or not) *and* that that synchronization is
external. If *one* of those 3 is done in IRQ context, then
all three should be, since they should be synchnornized wrt
each other.
Only idr_pre_get() should not be called from IRQ context.
*BUT* since idr_pre_get() and those other 3 may end up
in the same _internally_ locked region, _that_ internally
locked region should have the lowest common denominator lock,
_because_ of the other 3 which have to be syncrhonised wrt each other.
It is _this_ reason that the internal locking of IDR should use
use the lowest common denominator because of the context of
those other 3 which the _caller_ is responsible for synchronizing
depending on the caller's context.
Now James can we move on, please.
Andrew, please integrate this patch.
Thanks,
Luben
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.6.12.5 1/2] lib: allow idr to be used in irq context
2005-08-22 14:28 ` James Bottomley
@ 2005-08-22 16:51 ` Luben Tuikov
2005-08-22 21:53 ` James Bottomley
1 sibling, 0 replies; 18+ messages in thread
From: Luben Tuikov @ 2005-08-22 16:51 UTC (permalink / raw)
To: James Bottomley
Cc: Andrew Morton, jim.houston, Linux Kernel, SCSI Mailing List,
Dave Jones, Jeff Garzik
On 08/22/05 10:28, James Bottomley wrote:
> On Sun, 2005-08-21 at 20:52 -0700, Andrew Morton wrote:
>
>>erp. posix_timers has its own irq-safe lock, so we're doing extra,
>>unneeded locking in that code path.
>
>
> Possibly, the posix timer code is rather convoluted in this area so I'm
> not entirely sure my analysis is correct.
Then _please_ drop this thread.
>>I think providing locking inside idr.c was always a mistake - generally we
>>rely on caller-provided locking for such things.
>
>
> Well, the reason is because they wanted lockless pre-alloc. If you do
> it locked, you can't use GFP_KERNEL for the memory allocation flag which
> rather defeats its purpose.
James, please drop this thread and let's concentrate on SCSI.
> Perhaps the bug is in the API. We have pre-allocate, new, find and
> remove. Perhaps what we're missing is a reuse all of which could then
> rely on caller provided locking, with pre-alloc and remove requiring
> user context but new, find and reuse being happy in irq context.
No, the bug is _not_ in the API.
*IDR is perfect as it is.* It gives the caller a lot of freedom, without
internal restrictions (other than the provided by this patch).
Now please let's go back to SCSI and leave other people's code and API
alone. Shall we James?
SCSI Core needs enough work to have SCSI people worry about other
people's code and design... or is this the modus operandi of the
SCSI Core maintainer...? "Documentation/ManagementStyle", please?
Thank you,
Luben
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.6.12.5 1/2] lib: allow idr to be used in irq context
2005-08-22 14:28 ` James Bottomley
2005-08-22 16:51 ` Luben Tuikov
@ 2005-08-22 21:53 ` James Bottomley
2005-08-22 22:09 ` Andrew Morton
1 sibling, 1 reply; 18+ messages in thread
From: James Bottomley @ 2005-08-22 21:53 UTC (permalink / raw)
To: Andrew Morton
Cc: luben_tuikov, jim.houston, Linux Kernel, SCSI Mailing List,
Dave Jones, Jeff Garzik
On Mon, 2005-08-22 at 09:28 -0500, James Bottomley wrote:
> > I think providing locking inside idr.c was always a mistake - generally we
> > rely on caller-provided locking for such things.
>
> Well, the reason is because they wanted lockless pre-alloc. If you do
> it locked, you can't use GFP_KERNEL for the memory allocation flag which
> rather defeats its purpose.
It looks to be feasible to implement this locklessly the same way as the
radix-tree does (with a per_cpu list), except that you still need a
start/end API for the pre allocation to do the initial disable of pre-
emption.
Then the remove should simply then free the entry (again like radix-
tree) and let the slab take care of necessary locking.
Of course, if we're going to go to all this trouble, the next question
that arises naturally is why not just reuse the radix-tree code to
implement idr anyway ... ?
James
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.6.12.5 1/2] lib: allow idr to be used in irq context
2005-08-22 21:53 ` James Bottomley
@ 2005-08-22 22:09 ` Andrew Morton
2005-08-23 17:15 ` James Bottomley
0 siblings, 1 reply; 18+ messages in thread
From: Andrew Morton @ 2005-08-22 22:09 UTC (permalink / raw)
To: James Bottomley
Cc: luben_tuikov, jim.houston, linux-kernel, linux-scsi, davej,
jgarzik
James Bottomley <James.Bottomley@SteelEye.com> wrote:
>
> Of course, if we're going to go to all this trouble, the next question
> that arises naturally is why not just reuse the radix-tree code to
> implement idr anyway ... ?
Yes, we could probably have gone that way. radix-tree would need some
enhancements for the find-next-above thing.
radix-tree has some features (tags, gang-lookup, gang-lookup-by-tag) which
idr doesn't. Fitting them all into the one storage API would be nice, I
guess. radix-tree does potentially use more memory, although that'll only
be significant for collections which are both large and sparse.
Still, people can use either facility at present. The person who does any
such consolidation would do the kernel-wide migration at the same time.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.6.12.5 1/2] lib: allow idr to be used in irq context
2005-08-22 22:09 ` Andrew Morton
@ 2005-08-23 17:15 ` James Bottomley
0 siblings, 0 replies; 18+ messages in thread
From: James Bottomley @ 2005-08-23 17:15 UTC (permalink / raw)
To: Andrew Morton
Cc: luben_tuikov, jim.houston, Linux Kernel, SCSI Mailing List,
Dave Jones, Jeff Garzik
On Mon, 2005-08-22 at 15:09 -0700, Andrew Morton wrote:
> James Bottomley <James.Bottomley@SteelEye.com> wrote:
> >
> > Of course, if we're going to go to all this trouble, the next question
> > that arises naturally is why not just reuse the radix-tree code to
> > implement idr anyway ... ?
>
> Yes, we could probably have gone that way. radix-tree would need some
> enhancements for the find-next-above thing.
Yes, that's particularly simple (example patch below). However, radix-
tree needs to grow a bitmap index for whether its slots are occupied for
this to be made efficient (it would also help with the gang lookups
too)---at the moment it's O(N); it could be made O(log(N)).
> radix-tree has some features (tags, gang-lookup, gang-lookup-by-tag) which
> idr doesn't. Fitting them all into the one storage API would be nice, I
> guess. radix-tree does potentially use more memory, although that'll only
> be significant for collections which are both large and sparse.
And by definition, idr is trying to discourage sparsity in the
collections.
> Still, people can use either facility at present. The person who does any
> such consolidation would do the kernel-wide migration at the same time.
True. We only seem to have 8 in tree users ... that's not a huge number
to convert.
James
diff --git a/include/linux/radix-tree.h b/include/linux/radix-tree.h
--- a/include/linux/radix-tree.h
+++ b/include/linux/radix-tree.h
@@ -62,10 +62,20 @@ unsigned int
radix_tree_gang_lookup_tag(struct radix_tree_root *root, void **results,
unsigned long first_index, unsigned int max_items, int tag);
int radix_tree_tagged(struct radix_tree_root *root, int tag);
+int radix_tree_get_new_above(struct radix_tree_root *root, void *item,
+ unsigned long starting_index,
+ unsigned long *index);
static inline void radix_tree_preload_end(void)
{
preempt_enable();
}
+static inline int radix_tree_get_new(struct radix_tree_root *root, void *item,
+ unsigned long *index)
+{
+ return radix_tree_get_new_above(root, item, 0, index);
+}
+
+
#endif /* _LINUX_RADIX_TREE_H */
diff --git a/lib/radix-tree.c b/lib/radix-tree.c
--- a/lib/radix-tree.c
+++ b/lib/radix-tree.c
@@ -133,6 +133,7 @@ int radix_tree_preload(int gfp_mask)
out:
return ret;
}
+EXPORT_SYMBOL(radix_tree_preload);
static inline void tag_set(struct radix_tree_node *node, int tag, int offset)
{
@@ -483,7 +484,9 @@ __lookup(struct radix_tree_root *root, v
unsigned long i = (index >> shift) & RADIX_TREE_MAP_MASK;
for ( ; i < RADIX_TREE_MAP_SIZE; i++) {
- if (slot->slots[i] != NULL)
+ if (max_items == 0 && slot->slots[i] == NULL)
+ goto out;
+ else if (max_items != 0 && slot->slots[i] != NULL)
break;
index &= ~((1UL << shift) - 1);
index += 1UL << shift;
@@ -498,7 +501,9 @@ __lookup(struct radix_tree_root *root, v
for ( ; j < RADIX_TREE_MAP_SIZE; j++) {
index++;
- if (slot->slots[j]) {
+ if (max_items == 0 && slot->slots[j] == NULL)
+ goto out;
+ if (max_items != 0 && slot->slots[j]) {
results[nr_found++] = slot->slots[j];
if (nr_found == max_items)
goto out;
@@ -551,6 +556,28 @@ radix_tree_gang_lookup(struct radix_tree
}
EXPORT_SYMBOL(radix_tree_gang_lookup);
+/**
+ * radix_tree_get_new_above - insert item into first available slot
+ *
+ * @root: radix tree root
+ * @item: item to insert
+ * @starting_index: index to begin the search for a new slot from
+ * @index: actual index found for inserting the item
+ *
+ * Performs an index-ascending scan of the tree for an empty slot. Places
+ * @item in the first empty slot and sets @index to its position.
+ */
+int radix_tree_get_new_above(struct radix_tree_root *root, void *item,
+ unsigned long starting_index,
+ unsigned long *index)
+{
+ if (item == NULL)
+ item = (void *)1UL;
+ __lookup(root, NULL, starting_index, 0, index);
+ return radix_tree_insert(root, *index, item);
+}
+EXPORT_SYMBOL(radix_tree_get_new_above);
+
/*
* FIXME: the two tag_get()s here should use find_next_bit() instead of
* open-coding the search.
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2005-08-23 17:15 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-08-16 22:03 [PATCH 2.6.12.5 1/2] lib: allow idr to be used in irq context Luben Tuikov
2005-08-17 16:01 ` Jim Houston
2005-08-21 8:25 ` Andrew Morton
2005-08-21 15:49 ` Luben Tuikov
2005-08-21 16:06 ` James Bottomley
2005-08-21 17:27 ` Luben Tuikov
2005-08-21 22:03 ` James Bottomley
2005-08-22 0:33 ` Luben Tuikov
2005-08-22 3:15 ` James Bottomley
2005-08-22 3:52 ` Andrew Morton
2005-08-22 14:28 ` James Bottomley
2005-08-22 16:51 ` Luben Tuikov
2005-08-22 21:53 ` James Bottomley
2005-08-22 22:09 ` Andrew Morton
2005-08-23 17:15 ` James Bottomley
2005-08-22 16:33 ` Luben Tuikov
2005-08-22 14:06 ` Luben Tuikov
-- strict thread matches above, loose matches on Subject: below --
2005-08-21 20:40 Luben Tuikov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).