2008-06-10 10:24:16

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH] 2.6.26-rc: x86: pci-dma.c: use __GFP_NO_OOM instead of __GFP_NORETRY


* Miquel van Smoorenburg <[email protected]> wrote:

> On Mon, 2008-06-02 at 12:15 +0200, Ingo Molnar wrote:
> > * Miquel van Smoorenburg <[email protected]> wrote:
> >
> > > Okay, so how about this then ?
> >
> >
> > applied to tip/pci-for-jesse for more testing. Thanks,
>
> I've thought about it a bit more, and I think the actual patch that
> really does what everybody wants is this one instead:

applied a delta patch version of the one below to tip/pci-for-jesse.
Thanks,

Ingo

> [PATCH] x86: pci-dma.c: don't always add __GFP_NORETRY to gfp
>
> Currently arch/x86/kernel/pci-dma.c always adds __GFP_NORETRY
> to the allocation flags, because it wants to be reasonably
> sure not to deadlock when calling alloc_pages().
>
> But really that should only be done in two cases:
> - when allocating memory in the lower 16 MB DMA zone.
> If there's no free memory there, waiting or OOM killing is of no use
> - when optimistically trying an allocation in the DMA32 zone
> when dma_mask < DMA_32BIT_MASK hoping that the allocation
> happens to fall within the limits of the dma_mask
>
> Also blindly adding __GFP_NORETRY to the the gfp variable might
> not be a good idea since we then also use it when calling
> dma_ops->alloc_coherent(). Clearing it might also not be a
> good idea, dma_alloc_coherent()'s caller might have set it
> on purpose. The gfp variable should not be clobbered.
>
> Signed-off-by: Miquel van Smoorenburg <[email protected]>
>
> --- linux-2.6.26-rc4.orig/arch/x86/kernel/pci-dma.c 2008-05-26 20:08:11.000000000 +0200
> +++ linux-2.6.26-rc4/arch/x86/kernel/pci-dma.c 2008-06-05 17:51:41.000000000 +0200
> @@ -378,6 +378,7 @@ dma_alloc_coherent(struct device *dev, s
> struct page *page;
> unsigned long dma_mask = 0;
> dma_addr_t bus;
> + int noretry = 0;
>
> /* ignore region specifiers */
> gfp &= ~(__GFP_DMA | __GFP_HIGHMEM | __GFP_DMA32);
> @@ -397,20 +398,25 @@ dma_alloc_coherent(struct device *dev, s
> if (dev->dma_mask == NULL)
> return NULL;
>
> - /* Don't invoke OOM killer */
> - gfp |= __GFP_NORETRY;
> + /* Don't invoke OOM killer or retry in lower 16MB DMA zone */
> + if (gfp & __GFP_DMA)
> + noretry = 1;
>
> #ifdef CONFIG_X86_64
> /* Why <=? Even when the mask is smaller than 4GB it is often
> larger than 16MB and in this case we have a chance of
> finding fitting memory in the next higher zone first. If
> not retry with true GFP_DMA. -AK */
> - if (dma_mask <= DMA_32BIT_MASK && !(gfp & GFP_DMA))
> + if (dma_mask <= DMA_32BIT_MASK && !(gfp & GFP_DMA)) {
> gfp |= GFP_DMA32;
> + if (dma_mask < DMA_32BIT_MASK)
> + noretry = 1;
> + }
> #endif
>
> again:
> - page = dma_alloc_pages(dev, gfp, get_order(size));
> + page = dma_alloc_pages(dev,
> + noretry ? gfp | __GFP_NORETRY : gfp, get_order(size));
> if (page == NULL)
> return NULL;
>
>


2008-06-10 17:15:10

by Jesse Barnes

[permalink] [raw]
Subject: Re: [PATCH] 2.6.26-rc: x86: pci-dma.c: use __GFP_NO_OOM instead of __GFP_NORETRY

On Tuesday, June 10, 2008 3:23 am Ingo Molnar wrote:
> * Miquel van Smoorenburg <[email protected]> wrote:
> > On Mon, 2008-06-02 at 12:15 +0200, Ingo Molnar wrote:
> > > * Miquel van Smoorenburg <[email protected]> wrote:
> > > > Okay, so how about this then ?
> > >
> > > applied to tip/pci-for-jesse for more testing. Thanks,
> >
> > I've thought about it a bit more, and I think the actual patch that
> > really does what everybody wants is this one instead:
>
> applied a delta patch version of the one below to tip/pci-for-jesse.

Speaking of that branch, how do you want to handle it? Is it intended for my
linux-next branch or are some of the fixes important to get upstream now?
Which branch should I diff/log against to see the changes (last time I looked
it appeared neither master nor tip were the correct ones...)

Thanks,
Jesse

2008-06-11 14:26:55

by Miquel van Smoorenburg

[permalink] [raw]
Subject: Re: [PATCH] 2.6.26-rc: x86: pci-dma.c: use __GFP_NO_OOM instead of __GFP_NORETRY

On Tue, 2008-06-10 at 10:14 -0700, Jesse Barnes wrote:
> On Tuesday, June 10, 2008 3:23 am Ingo Molnar wrote:
> > * Miquel van Smoorenburg <[email protected]> wrote:
> > > On Mon, 2008-06-02 at 12:15 +0200, Ingo Molnar wrote:
> > > > * Miquel van Smoorenburg <[email protected]> wrote:
> > > > > Okay, so how about this then ?
> > > >
> > > > applied to tip/pci-for-jesse for more testing. Thanks,
> > >
> > > I've thought about it a bit more, and I think the actual patch that
> > > really does what everybody wants is this one instead:
> >
> > applied a delta patch version of the one below to tip/pci-for-jesse.
>
> Speaking of that branch, how do you want to handle it? Is it intended for my
> linux-next branch or are some of the fixes important to get upstream now?
> Which branch should I diff/log against to see the changes (last time I looked
> it appeared neither master nor tip were the correct ones...)

My opinion is that this particular patch should go into 2.6.26.

Mike.

2008-06-26 11:39:20

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH] 2.6.26-rc: x86: pci-dma.c: use __GFP_NO_OOM instead of __GFP_NORETRY


* Jesse Barnes <[email protected]> wrote:

> On Tuesday, June 10, 2008 3:23 am Ingo Molnar wrote:
> > * Miquel van Smoorenburg <[email protected]> wrote:
> > > On Mon, 2008-06-02 at 12:15 +0200, Ingo Molnar wrote:
> > > > * Miquel van Smoorenburg <[email protected]> wrote:
> > > > > Okay, so how about this then ?
> > > >
> > > > applied to tip/pci-for-jesse for more testing. Thanks,
> > >
> > > I've thought about it a bit more, and I think the actual patch that
> > > really does what everybody wants is this one instead:
> >
> > applied a delta patch version of the one below to tip/pci-for-jesse.
>
> Speaking of that branch, how do you want to handle it? Is it intended
> for my linux-next branch or are some of the fixes important to get
> upstream now? Which branch should I diff/log against to see the
> changes (last time I looked it appeared neither master nor tip were
> the correct ones...)

that branch is always based in -git so if stuff goes upstream patches
disappear from it. git-log linus/master..tip/pci-for-jesse should tell
the currently pending items. Right now it's just two lowprio items:

Yinghai Lu (2):
pci: debug extra pci resources range
pci: debug extra pci bus resources

Ingo

2008-07-02 02:40:16

by Jesse Barnes

[permalink] [raw]
Subject: Re: [PATCH] 2.6.26-rc: x86: pci-dma.c: use __GFP_NO_OOM instead of __GFP_NORETRY

On Thursday, June 26, 2008 4:38 am Ingo Molnar wrote:
> that branch is always based in -git so if stuff goes upstream patches
> disappear from it. git-log linus/master..tip/pci-for-jesse should tell
> the currently pending items. Right now it's just two lowprio items:
>
> Yinghai Lu (2):
> pci: debug extra pci resources range
> pci: debug extra pci bus resources

It looks like these add some new, unconditional debug output? I guess I
shouldn't worry about printks so much at this point, given how much
obsolete/uninteresting info gets dumped these days, but it would be nice if
this were only dumped if some debug option were set. Given the variety of
PCI stuff we might like to dump at various times, maybe it's time we added a
few new pci=debug=foo options? Yinghai?

Thanks,
Jesse

2008-07-02 02:46:25

by Yinghai Lu

[permalink] [raw]
Subject: Re: [PATCH] 2.6.26-rc: x86: pci-dma.c: use __GFP_NO_OOM instead of __GFP_NORETRY

On Tue, Jul 1, 2008 at 7:39 PM, Jesse Barnes <[email protected]> wrote:
> On Thursday, June 26, 2008 4:38 am Ingo Molnar wrote:
>> that branch is always based in -git so if stuff goes upstream patches
>> disappear from it. git-log linus/master..tip/pci-for-jesse should tell
>> the currently pending items. Right now it's just two lowprio items:
>>
>> Yinghai Lu (2):
>> pci: debug extra pci resources range
>> pci: debug extra pci bus resources
>
> It looks like these add some new, unconditional debug output? I guess I
> shouldn't worry about printks so much at this point, given how much
> obsolete/uninteresting info gets dumped these days, but it would be nice if
> this were only dumped if some debug option were set. Given the variety of
> PCI stuff we might like to dump at various times, maybe it's time we added a
> few new pci=debug=foo options? Yinghai?
>
that is just limited printout. and it is informative..
that is BAR before kernel try to assign new value to it.

YH

2008-07-02 02:49:12

by Jesse Barnes

[permalink] [raw]
Subject: Re: [PATCH] 2.6.26-rc: x86: pci-dma.c: use __GFP_NO_OOM instead of __GFP_NORETRY

On Tuesday, July 01, 2008 7:46 pm Yinghai Lu wrote:
> On Tue, Jul 1, 2008 at 7:39 PM, Jesse Barnes <[email protected]>
wrote:
> > On Thursday, June 26, 2008 4:38 am Ingo Molnar wrote:
> >> that branch is always based in -git so if stuff goes upstream patches
> >> disappear from it. git-log linus/master..tip/pci-for-jesse should tell
> >> the currently pending items. Right now it's just two lowprio items:
> >>
> >> Yinghai Lu (2):
> >> pci: debug extra pci resources range
> >> pci: debug extra pci bus resources
> >
> > It looks like these add some new, unconditional debug output? I guess I
> > shouldn't worry about printks so much at this point, given how much
> > obsolete/uninteresting info gets dumped these days, but it would be nice
> > if this were only dumped if some debug option were set. Given the
> > variety of PCI stuff we might like to dump at various times, maybe it's
> > time we added a few new pci=debug=foo options? Yinghai?
>
> that is just limited printout. and it is informative..
> that is BAR before kernel try to assign new value to it.

Yeah, and it's in keeping with the early (pre-assignment) BAR dumping code you
added elsewhere. I'm fine with it as-is, I'll revisit finer grained debug
options in a future release and fix things up then as needed.

Thanks,
Jesse