From: Robin Murphy <robin.murphy@arm.com>
To: Christoph Hellwig <hch@lst.de>
Cc: m.szyprowski@samsung.com, iommu@lists.linux-foundation.org,
linux-kernel@vger.kernel.org, cai@gmx.us, salil.mehta@huawei.com,
john.garry@huawei.com
Subject: Re: [PATCH 3/4] dma-debug: Dynamically expand the dma_debug_entry pool
Date: Tue, 4 Dec 2018 16:32:03 +0000 [thread overview]
Message-ID: <dae44ed0-6b84-0ed3-87c2-97940576709d@arm.com> (raw)
In-Reply-To: <20181204142938.GC2767@lst.de>
On 04/12/2018 14:29, Christoph Hellwig wrote:
>> + for (retry_count = 0; ; retry_count++) {
>> + spin_lock_irqsave(&free_entries_lock, flags);
>> +
>> + if (num_free_entries > 0)
>> + break;
>>
>> spin_unlock_irqrestore(&free_entries_lock, flags);
>
> Taking a spinlock just to read a single integer value doesn't really
> help anything.
If the freelist is non-empty we break out with the lock still held in
order to actually allocate our entry - only if there are no free entries
left do we drop the lock in order to handle the failure. This much is
just the original logic shuffled around a bit (with the tweak that
testing num_free_entries seemed justifiably simpler than the original
list_empty() check).
>> +
>> + if (retry_count < DMA_DEBUG_DYNAMIC_RETRIES &&
>> + !prealloc_memory(DMA_DEBUG_DYNAMIC_ENTRIES))
>
> Don't we need GFP_ATOMIC here? Also why do we need the retries?
Ah, right, we may be outside our own spinlock, but of course the whole
DMA API call which got us here might be under someone else's and/or in a
non-sleeping context - I'll fix that.
The number of retries is just to bound the loop due to its inherent
raciness - since we drop the lock to create more entries, under
pathological conditions by the time we get back in to grab one they
could have all gone. 2 retries (well, strictly it's 1 try and 1 retry)
was an entirely arbitrary choice just to accommodate that happening very
occasionally by chance.
However, if the dynamic allocations need GFP_ATOMIC for external reasons
anyway, then I don't need the lock-juggling that invites that race in
the first place, and the whole loop disappears again. Neat!
Robin.
next prev parent reply other threads:[~2018-12-04 16:32 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-03 17:28 [PATCH 0/4] dma-debug: implement dynamic entry allocation Robin Murphy
2018-12-03 17:28 ` [PATCH 1/4] dma-debug: Use pr_fmt() Robin Murphy
2018-12-04 14:26 ` Christoph Hellwig
2018-12-04 17:35 ` Joe Perches
2018-12-03 17:28 ` [PATCH 2/4] dma-debug: Refactor dma_debug_entry allocation Robin Murphy
2018-12-04 14:27 ` Christoph Hellwig
2018-12-04 16:09 ` Robin Murphy
2018-12-03 17:28 ` [PATCH 3/4] dma-debug: Dynamically expand the dma_debug_entry pool Robin Murphy
2018-12-03 18:23 ` John Garry
2018-12-04 13:11 ` Robin Murphy
2018-12-04 14:17 ` Christoph Hellwig
2018-12-04 16:06 ` Robin Murphy
2018-12-04 16:30 ` John Garry
2018-12-04 17:19 ` Robin Murphy
2018-12-04 17:38 ` John Garry
2018-12-04 14:29 ` Christoph Hellwig
2018-12-04 16:32 ` Robin Murphy [this message]
2018-12-03 17:28 ` [RFC 4/4] dma-debug: Make leak-like behaviour apparent Robin Murphy
2018-12-04 14:31 ` Christoph Hellwig
2018-12-03 17:34 ` [PATCH 0/4] dma-debug: implement dynamic entry allocation Christoph Hellwig
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=dae44ed0-6b84-0ed3-87c2-97940576709d@arm.com \
--to=robin.murphy@arm.com \
--cc=cai@gmx.us \
--cc=hch@lst.de \
--cc=iommu@lists.linux-foundation.org \
--cc=john.garry@huawei.com \
--cc=linux-kernel@vger.kernel.org \
--cc=m.szyprowski@samsung.com \
--cc=salil.mehta@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).