lkml.org 
[lkml]   [2003]   [Oct]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [NFS] RE: [autofs] multiple servers per automount
Ian Kent wrote:

>On Wed, 15 Oct 2003, Ingo Oeser wrote:
>
>
>>In your patch you allocate inside the spinlock.
>>
>>
>
>Do you mean we don't want to sleep under the spin lock?
>Would a GFP_ATOMIC make a difference to the analysis?
>
>
Yes, sleeping within a spinlock is bad practice because it may
eventually deadlock. Pretend that the lock is taken, the call to
kmalloc is made, the mm system doesn't have any immidiately free memory
and through some flow of execution requires that a some pseudo-block
device backed filesystem needs to be mounted -> deadlock. I have no
idea if this is currently a likely scenario, however not sleeping within
a lock is 'The Right Thing' and should be avoided at all costs.

GFP_ATOMIC should be avoided in most circumstances, particularly in
environments where the code can be refactored to allow for the sleep.
It is less likely to find free memory atomically and is thus more likely
to fail.

>>I would suggest to do sth. like the following:
>>
>>void *local;
>>if (!unamed_dev_inuse) {
>> local = get_zeroed_page(GFP_KERNEL);
>>
>> if (!local)
>> return -ENOMEM;
>>}
>>
>>spinlock(&unamed_dev_lock);
>>mb();
>>if (!unamed_dev_inuse) {
>> unamed_dev_inuse = local;
>>
>> /* Used globally, don't free now */
>> local = NULL;
>>}
>>
>>/*
>> Do the lookup and alloc
>> */
>>
>>spinunlock(&unamed_dev_lock);
>>
>>/* Free page, because of race on allocation. */
>>if (local)
>> free_page(local);
>>
>>
>>Which will swap the pointers atomically and still alloc outside the
>>non-sleeping locking.
>>
>>
>
>As I said please give me a hint about your thinking here.
>And the use of a memory barrier as well ... umm?
>
>
>

Ingo's patch simply moved the allocation outside the spinlock.. See my
later patch about moving the allocation to and __init section, which is
probably the cleaner thing to do and doesn't require grabbing the page
and using it conditionally.

As for the mb(), I *thought* that a spinlock implied a memory barrier,
however I think he put it there because it solves the age-old badness of
double-checked locking (search google for good explanations of the badness).

--
Mike Waychison
Sun Microsystems, Inc.
1 (650) 352-5299 voice
1 (416) 202-8336 voice
mailto: Michael.Waychison@Sun.COM
http://www.sun.com

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
NOTICE: The opinions expressed in this email are held by me,
and may not represent the views of Sun Microsystems, Inc.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 13:58    [W:0.058 / U:1.312 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site