Re: [Xenomai-core] [Adeos-main] [COW-BUG] __alloc_pages called from atomic context

[Jan Kiszka <jan.kiszka@domain.hid>]

[-- Attachment #1: Type: text/plain, Size: 2678 bytes --]

Gilles Chanteperdrix wrote:
> On 8/8/07, Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org> wrote:
>> Jan Kiszka wrote:
>>  > Jan Kiszka wrote:
>>  > > Philippe Gerum wrote:
>>  > >> On Tue, 2007-08-07 at 17:09 +0200, Gilles Chanteperdrix wrote:
>>  > >>> On 8/7/07, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>>  > >>>> Gilles Chanteperdrix wrote:
>>  > >>>>> The fact that you are in a hurry should not be an excuse to propose a
>>  > >>>>> fix which is much worse than the bug itself.
>>  > >>>> Please explain.
>>  > >>> Using GFP_ATOMIC is not a good idea, because the kernel needs the
>>  > >>> GFP_ATOMIC pools for allocation that take place from really atomic
>>  > >>> contexts such as interruption handlers for instance. fork should not
>>  > >>> be an atomic context.
>>  > >>>
>>  > >>> So the only reason I see why you propose this patch is because you are
>>  > >>> in a hurry. Ok, before we fix this properly, you can use GFP_ATOMIC
>>  > >>> locally, but please do not make
>>  > >>> this an official patch.
>>  > >>>
>>  > >> I'm afraid you are both right. Draining pages from the atomic pool might
>>  > >> starve truely atomic contexts, and releasing the lock guarding the
>>  > >> pagetable pages for the source mm seems contradictory with what the
>>  > >> current code tries to achieve in holding it.
>>  > >>
>>  > >> Here is a patch preallocating the page for the cow-breaking code, which
>>  > >> relies on the lock breaking pattern already in place. Not exactly
>>  > >> pretty, slightly tested here (UP, spinlock debug), but looks ok so far.
>>  > >> Needs SMP testing before any attempt is made to merge it.
>>  > >
>>  > > Funny. I just finished my own patch of this kind and made it pass a
>>  > > basic COW stress test here. Find it below, it is very similar. Will
>>  > > now analyse yours to see which one looks nicer and/or is more correct.
>>  >
>>  > Yours is clearly nicer, but it required/allowed a few more cleanups.
>>  > Find my variant attached (against our local 2.6.20 kernel).
>>  >
>>  > From my POV: problem solved. 8)
>>
>> It seems you solved this problem before I even had a chance to have
>> a look at the code.
> 
> Finally looking at the I-pipe patch, I see something suspicious:
> the patch replaces a "return err" with a "return 0" in zeromap_pud_range.

Indeed, that looks fishy. Appears to be a broken combination of these
two commits:

http://www.denx.de/cgi-bin/gitweb.cgi?p=ipipe-2.6.git;a=commitdiff;h=48ac83704927dca23a28c50163eacb71758b646e
http://www.denx.de/cgi-bin/gitweb.cgi?p=ipipe-2.6.git;a=commitdiff;h=edebab28d3bced559c6be3b2de8045b83cc05d53

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]