DMA allocations from CMA and fatal_signal_pending check

[Florian Fainelli]

Hi Joonsoo,

On 10/31/2014 01:28 AM, Joonsoo Kim wrote:
> On Tue, Oct 28, 2014 at 12:08:46PM -0700, Florian Fainelli wrote:
>> Hello,
>>
>> While debugging why some dma_alloc_coherent() allocations where
>> returning NULL on our brcmstb platform, specifically with
>> drivers/net/ethernet/broadcom/bcmcsysport.c, I came across the
>> fatal_signal_pending() check in mm/page_alloc.c which is there.
>>
>> This driver calls dma_alloc_coherent(, GFP_KERNEL) which ends up making
>> a coherent allocation from a CMA region on our platform. Since that
>> allocation is allowed to sleep, and because we are in bcm_syport_open(),
>> executed from process context, a pending signal makes
>> dma_alloc_coherent() return NULL.
> 
> Hello, Florian.
> 
> fatal_signal_pending means that there is SIGKILL on that process.
> I guess that caller of dma_alloc_coherent() will die soon.
> In this case, why CMA should be succeed?

I agree that the CMA allocation should not be allowed to succeed, but
the dma_alloc_coherent() allocation should succeed. If we look at the
sysport driver, there are kmalloc() calls to initialize private
structures, those will succeed (except under high memory pressure), so
by the same token, a driver expects DMA allocations to succeed (unless
we are under high memory pressure)

What are we trying to solve exactly with the fatal_signal_pending()
check here? Are we just optimizing for the case where a process has
allocated from a CMA region to allow this region to be returned to the
pool of free pages when it gets killed? Could there be another mechanism
used to reclaim those pages if we know the process is getting killed anyway?

> 
>>
>> There are two ways I could fix this:
>>
>> - use a GFP_ATOMIC allocation, which would avoid this sensitivity to a
>> pending signal being fatal (we suffer from the same issue in
>> bcm_sysport_resume)
>>
>> - move the DMA coherent allocation before bcm_sysport_open(), in the
>> driver's probe function, but if the network interface is never used, we
>> would be waisting precious DMA coherent memory for nothing (it is only 4
>> bytes times 32 but still
> 
> I guess that it is okay that bcm_sysport_open() return -EINTR?

Well, not really. This driver is not an isolated case, there are tons of
other networking drivers that do exactly the same thing, and we do
expect these dma_alloc_* calls to succeed.

I think we would want to ignore the fatal_signal_pending() check for
allocations coming through the dma_alloc_* API, although I agree this
could be a tough one when they are done from process context.

Updating all drivers to switch to GFP_ATOMIC allocations is not a good
idea, since that would exhaust the atomic DMA coherent pool for no good
reason.

FYI, we are hitting the same problem during suspend/resume, if you are
unlucky enough the suspending process get interrupted, you can get a lot
of crashes from drivers that do not expect their dma_alloc_coherent()
allocation to be sensible to signals.
--
Florian