* cpupools and locking
@ 2010-05-04 18:51 George Dunlap
2010-05-04 21:52 ` Keir Fraser
2010-05-05 5:25 ` Juergen Gross
0 siblings, 2 replies; 5+ messages in thread
From: George Dunlap @ 2010-05-04 18:51 UTC (permalink / raw)
To: xen-devel, Juergen Gross, Keir Fraser
Something seems not quite right about the cpupool locking... in
xen/common/cpupool.c:cpupool_do_domctl(), the cpupool_lock is only
held during the find for several operations. Doesn't that mean that,
for instance, it's possible for someone to call CPUPOOL_OP_DESTROY,
while someone concurrently calls CPUPOOL_OP_INFO, such that in the
INFO case, the find succeeds, but the structure is shortly thereafter
freed by DESTROY, even though INFO code still has a pointer to it
which may be dereferenced? I don't see any reference counting... am I
missing something?
-George
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: cpupools and locking
2010-05-04 18:51 cpupools and locking George Dunlap
@ 2010-05-04 21:52 ` Keir Fraser
2010-05-05 5:25 ` Juergen Gross
1 sibling, 0 replies; 5+ messages in thread
From: Keir Fraser @ 2010-05-04 21:52 UTC (permalink / raw)
To: George Dunlap, xen-devel, Juergen Gross
On 04/05/2010 19:51, "George Dunlap" <George.Dunlap@eu.citrix.com> wrote:
> Something seems not quite right about the cpupool locking... in
> xen/common/cpupool.c:cpupool_do_domctl(), the cpupool_lock is only
> held during the find for several operations. Doesn't that mean that,
> for instance, it's possible for someone to call CPUPOOL_OP_DESTROY,
> while someone concurrently calls CPUPOOL_OP_INFO, such that in the
> INFO case, the find succeeds, but the structure is shortly thereafter
> freed by DESTROY, even though INFO code still has a pointer to it
> which may be dereferenced? I don't see any reference counting... am I
> missing something?
It certainly looks like "optimistic" concurrency control to me. :-)
-- Keir
> -George
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: cpupools and locking
2010-05-04 18:51 cpupools and locking George Dunlap
2010-05-04 21:52 ` Keir Fraser
@ 2010-05-05 5:25 ` Juergen Gross
2010-05-05 9:00 ` Keir Fraser
1 sibling, 1 reply; 5+ messages in thread
From: Juergen Gross @ 2010-05-05 5:25 UTC (permalink / raw)
To: George Dunlap, Keir Fraser; +Cc: xen-devel
On 05/04/2010 08:51 PM, George Dunlap wrote:
> Something seems not quite right about the cpupool locking... in
> xen/common/cpupool.c:cpupool_do_domctl(), the cpupool_lock is only
> held during the find for several operations. Doesn't that mean that,
> for instance, it's possible for someone to call CPUPOOL_OP_DESTROY,
> while someone concurrently calls CPUPOOL_OP_INFO, such that in the
> INFO case, the find succeeds, but the structure is shortly thereafter
> freed by DESTROY, even though INFO code still has a pointer to it
> which may be dereferenced? I don't see any reference counting... am I
> missing something?
cpupool_do_domctl is called always while the domctl lock is being held. Maybe
I should have added a comment to document this assumption.
Keir's patch to move the cpupool commands to the sysctl interface makes a
change of the locking mandatory. I'll setup a patch for this.
Juergen
--
Juergen Gross Principal Developer Operating Systems
TSP ES&S SWE OS6 Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions e-mail: juergen.gross@ts.fujitsu.com
Domagkstr. 28 Internet: ts.fujitsu.com
D-80807 Muenchen Company details: ts.fujitsu.com/imprint.html
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: cpupools and locking
2010-05-05 5:25 ` Juergen Gross
@ 2010-05-05 9:00 ` Keir Fraser
2010-05-05 11:07 ` Juergen Gross
0 siblings, 1 reply; 5+ messages in thread
From: Keir Fraser @ 2010-05-05 9:00 UTC (permalink / raw)
To: Juergen Gross, George Dunlap; +Cc: xen-devel
On 05/05/2010 06:25, "Juergen Gross" <juergen.gross@ts.fujitsu.com> wrote:
> cpupool_do_domctl is called always while the domctl lock is being held. Maybe
> I should have added a comment to document this assumption.
Yes please. Even better would be to explicitly add your own big lock in
cpupool.c, just for clarity. And why do you need your 'little' lock around
the lookup operations at all, in that case? It seems unnecessary if you have
a bigger lock protecting you; yet insufficient if you do not.
> Keir's patch to move the cpupool commands to the sysctl interface makes a
> change of the locking mandatory. I'll setup a patch for this.
Well, there is a big sysctl_lock too, so your cpupool commands will still be
serialised. If you require serialisation against *other* domctl operations
then yes, you now have an issue. You could just grab the domctl lock of
course (no current ordering constraints on the two locks, and there is an
API for acquiring/releasing the domctl lock).
-- Keir
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: cpupools and locking
2010-05-05 9:00 ` Keir Fraser
@ 2010-05-05 11:07 ` Juergen Gross
0 siblings, 0 replies; 5+ messages in thread
From: Juergen Gross @ 2010-05-05 11:07 UTC (permalink / raw)
To: Keir Fraser; +Cc: George Dunlap, xen-devel
[-- Attachment #1: Type: text/plain, Size: 989 bytes --]
On 05/05/2010 11:00 AM, Keir Fraser wrote:
> On 05/05/2010 06:25, "Juergen Gross"<juergen.gross@ts.fujitsu.com> wrote:
>
>> cpupool_do_domctl is called always while the domctl lock is being held. Maybe
>> I should have added a comment to document this assumption.
>
> Yes please. Even better would be to explicitly add your own big lock in
> cpupool.c, just for clarity. And why do you need your 'little' lock around
> the lookup operations at all, in that case? It seems unnecessary if you have
> a bigger lock protecting you; yet insufficient if you do not.
That's the best solution, I think. Patch attached.
Juergen
--
Juergen Gross Principal Developer Operating Systems
TSP ES&S SWE OS6 Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions e-mail: juergen.gross@ts.fujitsu.com
Domagkstr. 28 Internet: ts.fujitsu.com
D-80807 Muenchen Company details: ts.fujitsu.com/imprint.html
[-- Attachment #2: poollock.patch --]
[-- Type: text/x-patch, Size: 1386 bytes --]
Signed-off-by: juergen.gross@ts.fujitsu.com
diff -r efa1b905d893 xen/common/cpupool.c
--- a/xen/common/cpupool.c Tue May 04 13:59:55 2010 +0100
+++ b/xen/common/cpupool.c Wed May 05 13:01:32 2010 +0200
@@ -37,6 +37,7 @@
* as it was obtained!
*/
static DEFINE_SPINLOCK(cpupool_lock);
+static DEFINE_SPINLOCK(cpupool_ctl_lock);
DEFINE_PER_CPU(struct cpupool *, cpupool);
@@ -401,6 +402,8 @@
int ret;
struct cpupool *c;
+ spin_lock(&cpupool_ctl_lock);
+
switch ( op->op )
{
@@ -426,9 +429,7 @@
case XEN_DOMCTL_CPUPOOL_OP_DESTROY:
{
- spin_lock(&cpupool_lock);
c = cpupool_find_by_id(op->cpupool_id, 1);
- spin_unlock(&cpupool_lock);
ret = -ENOENT;
if ( c == NULL )
break;
@@ -438,9 +439,7 @@
case XEN_DOMCTL_CPUPOOL_OP_INFO:
{
- spin_lock(&cpupool_lock);
c = cpupool_find_by_id(op->cpupool_id, 0);
- spin_unlock(&cpupool_lock);
ret = -ENOENT;
if ( c == NULL )
break;
@@ -484,9 +483,7 @@
{
unsigned cpu;
- spin_lock(&cpupool_lock);
c = cpupool_find_by_id(op->cpupool_id, 0);
- spin_unlock(&cpupool_lock);
ret = -ENOENT;
if ( c == NULL )
break;
@@ -560,6 +557,8 @@
}
+ spin_unlock(&cpupool_ctl_lock);
+
return ret;
}
[-- Attachment #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-05-05 11:07 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-05-04 18:51 cpupools and locking George Dunlap
2010-05-04 21:52 ` Keir Fraser
2010-05-05 5:25 ` Juergen Gross
2010-05-05 9:00 ` Keir Fraser
2010-05-05 11:07 ` Juergen Gross
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.