| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 10 Jun 2014 09:11:04 +0200
From: Vlastimil Babka <vbabka@...e.cz>
To: David Rientjes <rientjes@...gle.com>
CC: linux-mm@...ck.org, linux-kernel@...r.kernel.org,
Andrew Morton <akpm@...ux-foundation.org>,
Greg Thelen <gthelen@...gle.com>,
Minchan Kim <minchan@...nel.org>, Mel Gorman <mgorman@...e.de>,
Joonsoo Kim <iamjoonsoo.kim@....com>,
Michal Nazarewicz <mina86@...a86.com>,
Naoya Horiguchi <n-horiguchi@...jp.nec.com>,
Christoph Lameter <cl@...ux.com>,
Rik van Riel <riel@...hat.com>
Subject: Re: [PATCH 02/10] mm, compaction: report compaction as contended
only due to lock contention
On 06/10/2014 01:50 AM, David Rientjes wrote:
> On Mon, 9 Jun 2014, Vlastimil Babka wrote:
>
>> Async compaction aborts when it detects zone lock contention or need_resched()
>> is true. David Rientjes has reported that in practice, most direct async
>> compactions for THP allocation abort due to need_resched(). This means that a
>> second direct compaction is never attempted, which might be OK for a page
>> fault, but hugepaged is intended to attempt a sync compaction in such case and
>> in these cases it won't.
>>
>> This patch replaces "bool contended" in compact_control with an enum that
>> distinguieshes between aborting due to need_resched() and aborting due to lock
>> contention. This allows propagating the abort through all compaction functions
>> as before, but declaring the direct compaction as contended only when lock
>> contantion has been detected.
>>
>> As a result, hugepaged will proceed with second sync compaction as intended,
>> when the preceding async compaction aborted due to need_resched().
>>
>
> s/hugepaged/khugepaged/ on the changelog.
>
>> Reported-by: David Rientjes <rientjes@...gle.com>
>> Signed-off-by: Vlastimil Babka <vbabka@...e.cz>
>> Cc: Minchan Kim <minchan@...nel.org>
>> Cc: Mel Gorman <mgorman@...e.de>
>> Cc: Joonsoo Kim <iamjoonsoo.kim@....com>
>> Cc: Michal Nazarewicz <mina86@...a86.com>
>> Cc: Naoya Horiguchi <n-horiguchi@...jp.nec.com>
>> Cc: Christoph Lameter <cl@...ux.com>
>> Cc: Rik van Riel <riel@...hat.com>
>> ---
>> mm/compaction.c | 20 ++++++++++++++------
>> mm/internal.h | 15 +++++++++++----
>> 2 files changed, 25 insertions(+), 10 deletions(-)
>>
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index b73b182..d37f4a8 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -185,9 +185,14 @@ static void update_pageblock_skip(struct compact_control *cc,
>> }
>> #endif /* CONFIG_COMPACTION */
>>
>> -static inline bool should_release_lock(spinlock_t *lock)
>> +enum compact_contended should_release_lock(spinlock_t *lock)
>> {
>> - return need_resched() || spin_is_contended(lock);
>> + if (need_resched())
>> + return COMPACT_CONTENDED_SCHED;
>> + else if (spin_is_contended(lock))
>> + return COMPACT_CONTENDED_LOCK;
>> + else
>> + return COMPACT_CONTENDED_NONE;
>> }
>>
>> /*
>
> I think eventually we're going to remove the need_resched() heuristic
> entirely and so enum compact_contended might be overkill, but do we need
> to worry about spin_is_contended(lock) && need_resched() reporting
> COMPACT_CONTENDED_SCHED here instead of COMPACT_CONTENDED_LOCK?
Hm right, maybe I should reorder the two tests.
>> @@ -202,7 +207,9 @@ static inline bool should_release_lock(spinlock_t *lock)
>> static bool compact_checklock_irqsave(spinlock_t *lock, unsigned long *flags,
>> bool locked, struct compact_control *cc)
>> {
>> - if (should_release_lock(lock)) {
>> + enum compact_contended contended = should_release_lock(lock);
>> +
>> + if (contended) {
>> if (locked) {
>> spin_unlock_irqrestore(lock, *flags);
>> locked = false;
>> @@ -210,7 +217,7 @@ static bool compact_checklock_irqsave(spinlock_t *lock, unsigned long *flags,
>>
>> /* async aborts if taking too long or contended */
>> if (cc->mode == MIGRATE_ASYNC) {
>> - cc->contended = true;
>> + cc->contended = contended;
>> return false;
>> }
>>
>> @@ -236,7 +243,7 @@ static inline bool compact_should_abort(struct compact_control *cc)
>> /* async compaction aborts if contended */
>> if (need_resched()) {
>> if (cc->mode == MIGRATE_ASYNC) {
>> - cc->contended = true;
>> + cc->contended = COMPACT_CONTENDED_SCHED;
>> return true;
>> }
>>
>> @@ -1095,7 +1102,8 @@ static unsigned long compact_zone_order(struct zone *zone, int order,
>> VM_BUG_ON(!list_empty(&cc.freepages));
>> VM_BUG_ON(!list_empty(&cc.migratepages));
>>
>> - *contended = cc.contended;
>> + /* We only signal lock contention back to the allocator */
>> + *contended = cc.contended == COMPACT_CONTENDED_LOCK;
>> return ret;
>> }
>>
>
> Hmm, since the only thing that matters for cc->contended is
> COMPACT_CONTENDED_LOCK, it may make sense to just leave this as a bool
> within struct compact_control instead of passing the actual reason around
> when it doesn't matter.
That's what I thought first. But we set cc->contended in
isolate_freepages_block() and then check it in isolate_freepages() and
compaction_alloc() to make sure we don't continue the free scanner once
contention (or need_resched()) is detected. And introducing an enum,
even if temporary measure, seemed simpler than making that checking more
complex. This way it can stay the same once we get rid of need_resched().
>> diff --git a/mm/internal.h b/mm/internal.h
>> index 7f22a11f..4659e8e 100644
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -117,6 +117,13 @@ extern int user_min_free_kbytes;
>>
>> #if defined CONFIG_COMPACTION || defined CONFIG_CMA
>>
>> +/* Used to signal whether compaction detected need_sched() or lock contention */
>> +enum compact_contended {
>> + COMPACT_CONTENDED_NONE = 0, /* no contention detected */
>> + COMPACT_CONTENDED_SCHED, /* need_sched() was true */
>> + COMPACT_CONTENDED_LOCK, /* zone lock or lru_lock was contended */
>> +};
>> +
>> /*
>> * in mm/compaction.c
>> */
>> @@ -144,10 +151,10 @@ struct compact_control {
>> int order; /* order a direct compactor needs */
>> int migratetype; /* MOVABLE, RECLAIMABLE etc */
>> struct zone *zone;
>> - bool contended; /* True if a lock was contended, or
>> - * need_resched() true during async
>> - * compaction
>> - */
>> + enum compact_contended contended; /* Signal need_sched() or lock
>> + * contention detected during
>> + * compaction
>> + */
>> };
>>
>> unsigned long
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists