* [PATCH] lib/ida: Document locking requirements a bit better
@ 2016-10-26 14:27 ` Daniel Vetter
0 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2016-10-26 14:27 UTC (permalink / raw)
To: LKML
Cc: Intel Graphics Development, Daniel Vetter, Mel Gorman,
Michal Hocko, Vlastimil Babka, Tejun Heo, Andrew Morton,
Daniel Vetter
I wanted to wrap a bunch of ida_simple_get calls into their own
locking, until I dug around and read the original commit message.
Stuff like this should imo be added to the kernel doc, let's do that.
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Tejun Heo <tj@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
lib/idr.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/lib/idr.c b/lib/idr.c
index 6098336df267..5508d7f6d7be 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -927,6 +927,9 @@ EXPORT_SYMBOL(ida_pre_get);
* and go back to the ida_pre_get() call. If the ida is full, it will
* return %-ENOSPC.
*
+ * Note that callers must ensure that concurrent access to @ida is not possible.
+ * When simplicity trumps concurrency needs look at ida_simple_get() instead.
+ *
* @p_id returns a value in the range @starting_id ... %0x7fffffff.
*/
int ida_get_new_above(struct ida *ida, int starting_id, int *p_id)
@@ -1073,6 +1076,10 @@ EXPORT_SYMBOL(ida_destroy);
* Allocates an id in the range start <= id < end, or returns -ENOSPC.
* On memory allocation failure, returns -ENOMEM.
*
+ * Compared to ida_get_new_above() this function does its own locking and hence
+ * is recommended everywhere where simplicity is preferred over the last bit of
+ * speed.
+ *
* Use ida_simple_remove() to get rid of an id.
*/
int ida_simple_get(struct ida *ida, unsigned int start, unsigned int end,
@@ -1119,6 +1126,13 @@ EXPORT_SYMBOL(ida_simple_get);
* ida_simple_remove - remove an allocated id.
* @ida: the (initialized) ida.
* @id: the id returned by ida_simple_get.
+ *
+ * Use to release an id allocated with ida_simple_get().
+ *
+ * Compared to ida_get_new_above() this function does its own locking and hence
+ * is recommended everywhere where simplicity is preferred over the last bit of
+ * speed.
+ *
*/
void ida_simple_remove(struct ida *ida, unsigned int id)
{
--
2.10.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH] lib/ida: Document locking requirements a bit better
@ 2016-10-26 14:27 ` Daniel Vetter
0 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2016-10-26 14:27 UTC (permalink / raw)
To: LKML
Cc: Michal Hocko, Daniel Vetter, Intel Graphics Development,
Tejun Heo, Daniel Vetter, Andrew Morton, Mel Gorman,
Vlastimil Babka
I wanted to wrap a bunch of ida_simple_get calls into their own
locking, until I dug around and read the original commit message.
Stuff like this should imo be added to the kernel doc, let's do that.
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Tejun Heo <tj@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
lib/idr.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/lib/idr.c b/lib/idr.c
index 6098336df267..5508d7f6d7be 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -927,6 +927,9 @@ EXPORT_SYMBOL(ida_pre_get);
* and go back to the ida_pre_get() call. If the ida is full, it will
* return %-ENOSPC.
*
+ * Note that callers must ensure that concurrent access to @ida is not possible.
+ * When simplicity trumps concurrency needs look at ida_simple_get() instead.
+ *
* @p_id returns a value in the range @starting_id ... %0x7fffffff.
*/
int ida_get_new_above(struct ida *ida, int starting_id, int *p_id)
@@ -1073,6 +1076,10 @@ EXPORT_SYMBOL(ida_destroy);
* Allocates an id in the range start <= id < end, or returns -ENOSPC.
* On memory allocation failure, returns -ENOMEM.
*
+ * Compared to ida_get_new_above() this function does its own locking and hence
+ * is recommended everywhere where simplicity is preferred over the last bit of
+ * speed.
+ *
* Use ida_simple_remove() to get rid of an id.
*/
int ida_simple_get(struct ida *ida, unsigned int start, unsigned int end,
@@ -1119,6 +1126,13 @@ EXPORT_SYMBOL(ida_simple_get);
* ida_simple_remove - remove an allocated id.
* @ida: the (initialized) ida.
* @id: the id returned by ida_simple_get.
+ *
+ * Use to release an id allocated with ida_simple_get().
+ *
+ * Compared to ida_get_new_above() this function does its own locking and hence
+ * is recommended everywhere where simplicity is preferred over the last bit of
+ * speed.
+ *
*/
void ida_simple_remove(struct ida *ida, unsigned int id)
{
--
2.10.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] lib/ida: Document locking requirements a bit better
2016-10-26 14:27 ` Daniel Vetter
@ 2016-10-26 14:39 ` Tejun Heo
-1 siblings, 0 replies; 11+ messages in thread
From: Tejun Heo @ 2016-10-26 14:39 UTC (permalink / raw)
To: Daniel Vetter
Cc: LKML, Intel Graphics Development, Mel Gorman, Michal Hocko,
Vlastimil Babka, Andrew Morton, Daniel Vetter
Hello, Daniel.
On Wed, Oct 26, 2016 at 04:27:39PM +0200, Daniel Vetter wrote:
> I wanted to wrap a bunch of ida_simple_get calls into their own
> locking, until I dug around and read the original commit message.
> Stuff like this should imo be added to the kernel doc, let's do that.
Generally agreed but some nits below.
> @@ -927,6 +927,9 @@ EXPORT_SYMBOL(ida_pre_get);
> * and go back to the ida_pre_get() call. If the ida is full, it will
> * return %-ENOSPC.
> *
> + * Note that callers must ensure that concurrent access to @ida is not possible.
> + * When simplicity trumps concurrency needs look at ida_simple_get() instead.
Maybe we can make it a bit less dramatic?
> @@ -1073,6 +1076,10 @@ EXPORT_SYMBOL(ida_destroy);
> * Allocates an id in the range start <= id < end, or returns -ENOSPC.
> * On memory allocation failure, returns -ENOMEM.
> *
> + * Compared to ida_get_new_above() this function does its own locking and hence
> + * is recommended everywhere where simplicity is preferred over the last bit of
> + * speed.
Hmm... so, this isn't necessarily about speed. For example, id
allocation might have to happen inside a spinlock which protects a
larger scope. To guarantee GFP_KERNEL allocation behavior in such
cases, the caller would have to call ida_pre_get() outside the said
spinlock and then call ida_get_new_above() inside the lock.
I think it'd be better to explain what the simple version does and
expects and then say that unless there are specific requirements using
the simple version is recommended.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] lib/ida: Document locking requirements a bit better
@ 2016-10-26 14:39 ` Tejun Heo
0 siblings, 0 replies; 11+ messages in thread
From: Tejun Heo @ 2016-10-26 14:39 UTC (permalink / raw)
To: Daniel Vetter
Cc: Michal Hocko, Intel Graphics Development, LKML, Daniel Vetter,
Andrew Morton, Mel Gorman, Vlastimil Babka
Hello, Daniel.
On Wed, Oct 26, 2016 at 04:27:39PM +0200, Daniel Vetter wrote:
> I wanted to wrap a bunch of ida_simple_get calls into their own
> locking, until I dug around and read the original commit message.
> Stuff like this should imo be added to the kernel doc, let's do that.
Generally agreed but some nits below.
> @@ -927,6 +927,9 @@ EXPORT_SYMBOL(ida_pre_get);
> * and go back to the ida_pre_get() call. If the ida is full, it will
> * return %-ENOSPC.
> *
> + * Note that callers must ensure that concurrent access to @ida is not possible.
> + * When simplicity trumps concurrency needs look at ida_simple_get() instead.
Maybe we can make it a bit less dramatic?
> @@ -1073,6 +1076,10 @@ EXPORT_SYMBOL(ida_destroy);
> * Allocates an id in the range start <= id < end, or returns -ENOSPC.
> * On memory allocation failure, returns -ENOMEM.
> *
> + * Compared to ida_get_new_above() this function does its own locking and hence
> + * is recommended everywhere where simplicity is preferred over the last bit of
> + * speed.
Hmm... so, this isn't necessarily about speed. For example, id
allocation might have to happen inside a spinlock which protects a
larger scope. To guarantee GFP_KERNEL allocation behavior in such
cases, the caller would have to call ida_pre_get() outside the said
spinlock and then call ida_get_new_above() inside the lock.
I think it'd be better to explain what the simple version does and
expects and then say that unless there are specific requirements using
the simple version is recommended.
Thanks.
--
tejun
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* ✓ Fi.CI.BAT: success for lib/ida: Document locking requirements a bit better
2016-10-26 14:27 ` Daniel Vetter
(?)
(?)
@ 2016-10-26 14:45 ` Patchwork
-1 siblings, 0 replies; 11+ messages in thread
From: Patchwork @ 2016-10-26 14:45 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
== Series Details ==
Series: lib/ida: Document locking requirements a bit better
URL : https://patchwork.freedesktop.org/series/14410/
State : success
== Summary ==
Series 14410v1 lib/ida: Document locking requirements a bit better
https://patchwork.freedesktop.org/api/1.0/series/14410/revisions/1/mbox/
fi-bdw-5557u total:246 pass:231 dwarn:0 dfail:0 fail:0 skip:15
fi-bxt-t5700 total:246 pass:216 dwarn:0 dfail:0 fail:0 skip:30
fi-byt-j1900 total:246 pass:215 dwarn:0 dfail:0 fail:0 skip:31
fi-byt-n2820 total:246 pass:211 dwarn:0 dfail:0 fail:0 skip:35
fi-hsw-4770 total:246 pass:224 dwarn:0 dfail:0 fail:0 skip:22
fi-hsw-4770r total:246 pass:223 dwarn:0 dfail:0 fail:0 skip:23
fi-ilk-650 total:246 pass:185 dwarn:0 dfail:0 fail:0 skip:61
fi-ivb-3520m total:246 pass:220 dwarn:0 dfail:0 fail:0 skip:26
fi-ivb-3770 total:246 pass:220 dwarn:0 dfail:0 fail:0 skip:26
fi-kbl-7200u total:246 pass:222 dwarn:0 dfail:0 fail:0 skip:24
fi-skl-6260u total:246 pass:232 dwarn:0 dfail:0 fail:0 skip:14
fi-skl-6700hq total:246 pass:223 dwarn:0 dfail:0 fail:0 skip:23
fi-skl-6700k total:246 pass:222 dwarn:1 dfail:0 fail:0 skip:23
fi-skl-6770hq total:246 pass:232 dwarn:0 dfail:0 fail:0 skip:14
fi-snb-2520m total:246 pass:209 dwarn:0 dfail:0 fail:0 skip:37
fi-snb-2600 total:246 pass:208 dwarn:0 dfail:0 fail:0 skip:38
fi-bsw-n3050 failed to connect after reboot
760634d2b78657f0a2267f73bff0d527f6c69ce5 drm-intel-nightly: 2016y-10m-26d-11h-52m-53s UTC integration manifest
a8f9106 lib/ida: Document locking requirements a bit better
Full results at https://intel-gfx-ci.01.org/CI/Patchwork_2824/
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2824/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] lib/ida: Document locking requirements a bit better
2016-10-26 14:39 ` Tejun Heo
@ 2016-10-26 19:25 ` Daniel Vetter
-1 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2016-10-26 19:25 UTC (permalink / raw)
To: Tejun Heo
Cc: Daniel Vetter, LKML, Intel Graphics Development, Mel Gorman,
Michal Hocko, Vlastimil Babka, Andrew Morton, Daniel Vetter
On Wed, Oct 26, 2016 at 10:39:29AM -0400, Tejun Heo wrote:
> Hello, Daniel.
>
> On Wed, Oct 26, 2016 at 04:27:39PM +0200, Daniel Vetter wrote:
> > I wanted to wrap a bunch of ida_simple_get calls into their own
> > locking, until I dug around and read the original commit message.
> > Stuff like this should imo be added to the kernel doc, let's do that.
>
> Generally agreed but some nits below.
I value good docs but I suck at typing them ;-)
> > @@ -927,6 +927,9 @@ EXPORT_SYMBOL(ida_pre_get);
> > * and go back to the ida_pre_get() call. If the ida is full, it will
> > * return %-ENOSPC.
> > *
> > + * Note that callers must ensure that concurrent access to @ida is not possible.
> > + * When simplicity trumps concurrency needs look at ida_simple_get() instead.
>
> Maybe we can make it a bit less dramatic?
What about?
"Note that callers must ensure that concurrent access to @ida is not possible.
See ida_simple_get() for a varaint which takes care of locking.
>
>
> > @@ -1073,6 +1076,10 @@ EXPORT_SYMBOL(ida_destroy);
> > * Allocates an id in the range start <= id < end, or returns -ENOSPC.
> > * On memory allocation failure, returns -ENOMEM.
> > *
> > + * Compared to ida_get_new_above() this function does its own locking and hence
> > + * is recommended everywhere where simplicity is preferred over the last bit of
> > + * speed.
>
> Hmm... so, this isn't necessarily about speed. For example, id
> allocation might have to happen inside a spinlock which protects a
> larger scope. To guarantee GFP_KERNEL allocation behavior in such
> cases, the caller would have to call ida_pre_get() outside the said
> spinlock and then call ida_get_new_above() inside the lock.
Hm, ida_simple_get does that for you already ...
> I think it'd be better to explain what the simple version does and
> expects and then say that unless there are specific requirements using
> the simple version is recommended.
What about:
"Compared to ida_get_new_above() this function does its own locking, and
should be used unless there are special requirements."
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] lib/ida: Document locking requirements a bit better
@ 2016-10-26 19:25 ` Daniel Vetter
0 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2016-10-26 19:25 UTC (permalink / raw)
To: Tejun Heo
Cc: Michal Hocko, Daniel Vetter, Intel Graphics Development, LKML,
Daniel Vetter, Andrew Morton, Mel Gorman, Vlastimil Babka
On Wed, Oct 26, 2016 at 10:39:29AM -0400, Tejun Heo wrote:
> Hello, Daniel.
>
> On Wed, Oct 26, 2016 at 04:27:39PM +0200, Daniel Vetter wrote:
> > I wanted to wrap a bunch of ida_simple_get calls into their own
> > locking, until I dug around and read the original commit message.
> > Stuff like this should imo be added to the kernel doc, let's do that.
>
> Generally agreed but some nits below.
I value good docs but I suck at typing them ;-)
> > @@ -927,6 +927,9 @@ EXPORT_SYMBOL(ida_pre_get);
> > * and go back to the ida_pre_get() call. If the ida is full, it will
> > * return %-ENOSPC.
> > *
> > + * Note that callers must ensure that concurrent access to @ida is not possible.
> > + * When simplicity trumps concurrency needs look at ida_simple_get() instead.
>
> Maybe we can make it a bit less dramatic?
What about?
"Note that callers must ensure that concurrent access to @ida is not possible.
See ida_simple_get() for a varaint which takes care of locking.
>
>
> > @@ -1073,6 +1076,10 @@ EXPORT_SYMBOL(ida_destroy);
> > * Allocates an id in the range start <= id < end, or returns -ENOSPC.
> > * On memory allocation failure, returns -ENOMEM.
> > *
> > + * Compared to ida_get_new_above() this function does its own locking and hence
> > + * is recommended everywhere where simplicity is preferred over the last bit of
> > + * speed.
>
> Hmm... so, this isn't necessarily about speed. For example, id
> allocation might have to happen inside a spinlock which protects a
> larger scope. To guarantee GFP_KERNEL allocation behavior in such
> cases, the caller would have to call ida_pre_get() outside the said
> spinlock and then call ida_get_new_above() inside the lock.
Hm, ida_simple_get does that for you already ...
> I think it'd be better to explain what the simple version does and
> expects and then say that unless there are specific requirements using
> the simple version is recommended.
What about:
"Compared to ida_get_new_above() this function does its own locking, and
should be used unless there are special requirements."
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] lib/ida: Document locking requirements a bit better
2016-10-26 19:25 ` Daniel Vetter
@ 2016-10-26 20:07 ` Tejun Heo
-1 siblings, 0 replies; 11+ messages in thread
From: Tejun Heo @ 2016-10-26 20:07 UTC (permalink / raw)
To: Daniel Vetter
Cc: Daniel Vetter, LKML, Intel Graphics Development, Mel Gorman,
Michal Hocko, Vlastimil Babka, Andrew Morton, Daniel Vetter
Hello, Daniel.
On Wed, Oct 26, 2016 at 09:25:25PM +0200, Daniel Vetter wrote:
> > > + * Note that callers must ensure that concurrent access to @ida is not possible.
> > > + * When simplicity trumps concurrency needs look at ida_simple_get() instead.
> >
> > Maybe we can make it a bit less dramatic?
>
> What about?
>
> "Note that callers must ensure that concurrent access to @ida is not possible.
> See ida_simple_get() for a varaint which takes care of locking.
Yeah, that reads easier to me.
> > Hmm... so, this isn't necessarily about speed. For example, id
> > allocation might have to happen inside a spinlock which protects a
> > larger scope. To guarantee GFP_KERNEL allocation behavior in such
> > cases, the caller would have to call ida_pre_get() outside the said
> > spinlock and then call ida_get_new_above() inside the lock.
>
> Hm, ida_simple_get does that for you already ...
Here's an example.
spin_lock();
do some stuff;
something->id = ida_simple_get(some gfp flag);
do some stuff;
spin_unlock();
In this scenario, you can't use sleeping GFPs for ida_simple_get()
because it does preloading inside it. What one has to do is...
ida_pre_get(GFP_KERNEL);
spin_lock();
do some stuff;
something->id = ida_get_new_above(GFP_NOWAIT);
do some stuff;
spin_unlock();
So, I guess it can be sometimes about avoiding the extra locking
overhead but it's more often about separating out allocation context
into an earlier call.
> > I think it'd be better to explain what the simple version does and
> > expects and then say that unless there are specific requirements using
> > the simple version is recommended.
>
> What about:
>
> "Compared to ida_get_new_above() this function does its own locking, and
> should be used unless there are special requirements."
Yeah, looks good to me.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] lib/ida: Document locking requirements a bit better
@ 2016-10-26 20:07 ` Tejun Heo
0 siblings, 0 replies; 11+ messages in thread
From: Tejun Heo @ 2016-10-26 20:07 UTC (permalink / raw)
To: Daniel Vetter
Cc: Michal Hocko, Daniel Vetter, Intel Graphics Development, LKML,
Daniel Vetter, Andrew Morton, Mel Gorman, Vlastimil Babka
Hello, Daniel.
On Wed, Oct 26, 2016 at 09:25:25PM +0200, Daniel Vetter wrote:
> > > + * Note that callers must ensure that concurrent access to @ida is not possible.
> > > + * When simplicity trumps concurrency needs look at ida_simple_get() instead.
> >
> > Maybe we can make it a bit less dramatic?
>
> What about?
>
> "Note that callers must ensure that concurrent access to @ida is not possible.
> See ida_simple_get() for a varaint which takes care of locking.
Yeah, that reads easier to me.
> > Hmm... so, this isn't necessarily about speed. For example, id
> > allocation might have to happen inside a spinlock which protects a
> > larger scope. To guarantee GFP_KERNEL allocation behavior in such
> > cases, the caller would have to call ida_pre_get() outside the said
> > spinlock and then call ida_get_new_above() inside the lock.
>
> Hm, ida_simple_get does that for you already ...
Here's an example.
spin_lock();
do some stuff;
something->id = ida_simple_get(some gfp flag);
do some stuff;
spin_unlock();
In this scenario, you can't use sleeping GFPs for ida_simple_get()
because it does preloading inside it. What one has to do is...
ida_pre_get(GFP_KERNEL);
spin_lock();
do some stuff;
something->id = ida_get_new_above(GFP_NOWAIT);
do some stuff;
spin_unlock();
So, I guess it can be sometimes about avoiding the extra locking
overhead but it's more often about separating out allocation context
into an earlier call.
> > I think it'd be better to explain what the simple version does and
> > expects and then say that unless there are specific requirements using
> > the simple version is recommended.
>
> What about:
>
> "Compared to ida_get_new_above() this function does its own locking, and
> should be used unless there are special requirements."
Yeah, looks good to me.
Thanks.
--
tejun
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] lib/ida: Document locking requirements a bit better
2016-10-26 20:07 ` Tejun Heo
@ 2016-10-27 7:19 ` Daniel Vetter
-1 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2016-10-27 7:19 UTC (permalink / raw)
To: Tejun Heo
Cc: Daniel Vetter, Daniel Vetter, LKML, Intel Graphics Development,
Mel Gorman, Michal Hocko, Vlastimil Babka, Andrew Morton,
Daniel Vetter
On Wed, Oct 26, 2016 at 04:07:25PM -0400, Tejun Heo wrote:
> Hello, Daniel.
>
> On Wed, Oct 26, 2016 at 09:25:25PM +0200, Daniel Vetter wrote:
> > > > + * Note that callers must ensure that concurrent access to @ida is not possible.
> > > > + * When simplicity trumps concurrency needs look at ida_simple_get() instead.
> > >
> > > Maybe we can make it a bit less dramatic?
> >
> > What about?
> >
> > "Note that callers must ensure that concurrent access to @ida is not possible.
> > See ida_simple_get() for a varaint which takes care of locking.
>
> Yeah, that reads easier to me.
>
> > > Hmm... so, this isn't necessarily about speed. For example, id
> > > allocation might have to happen inside a spinlock which protects a
> > > larger scope. To guarantee GFP_KERNEL allocation behavior in such
> > > cases, the caller would have to call ida_pre_get() outside the said
> > > spinlock and then call ida_get_new_above() inside the lock.
> >
> > Hm, ida_simple_get does that for you already ...
>
> Here's an example.
>
> spin_lock();
> do some stuff;
> something->id = ida_simple_get(some gfp flag);
> do some stuff;
> spin_unlock();
>
> In this scenario, you can't use sleeping GFPs for ida_simple_get()
> because it does preloading inside it. What one has to do is...
>
> ida_pre_get(GFP_KERNEL);
> spin_lock();
> do some stuff;
> something->id = ida_get_new_above(GFP_NOWAIT);
> do some stuff;
> spin_unlock();
>
> So, I guess it can be sometimes about avoiding the extra locking
> overhead but it's more often about separating out allocation context
> into an earlier call.
Hm yeah, calling ida_simple_get in that case isn't recommend really.
> > > I think it'd be better to explain what the simple version does and
> > > expects and then say that unless there are specific requirements using
> > > the simple version is recommended.
> >
> > What about:
> >
> > "Compared to ida_get_new_above() this function does its own locking, and
> > should be used unless there are special requirements."
>
> Yeah, looks good to me.
Ok, will respin, thanks for the review comments.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] lib/ida: Document locking requirements a bit better
@ 2016-10-27 7:19 ` Daniel Vetter
0 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2016-10-27 7:19 UTC (permalink / raw)
To: Tejun Heo
Cc: Michal Hocko, Daniel Vetter, Intel Graphics Development, LKML,
Daniel Vetter, Andrew Morton, Mel Gorman, Vlastimil Babka
On Wed, Oct 26, 2016 at 04:07:25PM -0400, Tejun Heo wrote:
> Hello, Daniel.
>
> On Wed, Oct 26, 2016 at 09:25:25PM +0200, Daniel Vetter wrote:
> > > > + * Note that callers must ensure that concurrent access to @ida is not possible.
> > > > + * When simplicity trumps concurrency needs look at ida_simple_get() instead.
> > >
> > > Maybe we can make it a bit less dramatic?
> >
> > What about?
> >
> > "Note that callers must ensure that concurrent access to @ida is not possible.
> > See ida_simple_get() for a varaint which takes care of locking.
>
> Yeah, that reads easier to me.
>
> > > Hmm... so, this isn't necessarily about speed. For example, id
> > > allocation might have to happen inside a spinlock which protects a
> > > larger scope. To guarantee GFP_KERNEL allocation behavior in such
> > > cases, the caller would have to call ida_pre_get() outside the said
> > > spinlock and then call ida_get_new_above() inside the lock.
> >
> > Hm, ida_simple_get does that for you already ...
>
> Here's an example.
>
> spin_lock();
> do some stuff;
> something->id = ida_simple_get(some gfp flag);
> do some stuff;
> spin_unlock();
>
> In this scenario, you can't use sleeping GFPs for ida_simple_get()
> because it does preloading inside it. What one has to do is...
>
> ida_pre_get(GFP_KERNEL);
> spin_lock();
> do some stuff;
> something->id = ida_get_new_above(GFP_NOWAIT);
> do some stuff;
> spin_unlock();
>
> So, I guess it can be sometimes about avoiding the extra locking
> overhead but it's more often about separating out allocation context
> into an earlier call.
Hm yeah, calling ida_simple_get in that case isn't recommend really.
> > > I think it'd be better to explain what the simple version does and
> > > expects and then say that unless there are specific requirements using
> > > the simple version is recommended.
> >
> > What about:
> >
> > "Compared to ida_get_new_above() this function does its own locking, and
> > should be used unless there are special requirements."
>
> Yeah, looks good to me.
Ok, will respin, thanks for the review comments.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-10-27 7:19 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-26 14:27 [PATCH] lib/ida: Document locking requirements a bit better Daniel Vetter
2016-10-26 14:27 ` Daniel Vetter
2016-10-26 14:39 ` Tejun Heo
2016-10-26 14:39 ` Tejun Heo
2016-10-26 19:25 ` Daniel Vetter
2016-10-26 19:25 ` Daniel Vetter
2016-10-26 20:07 ` Tejun Heo
2016-10-26 20:07 ` Tejun Heo
2016-10-27 7:19 ` Daniel Vetter
2016-10-27 7:19 ` Daniel Vetter
2016-10-26 14:45 ` ✓ Fi.CI.BAT: success for " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.