linux-kernel - Re: [PATCH 6/6] mm/slab: allocation fastpath without disabling irq

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150105172139.GA11201@rhlx01.hs-esslingen.de>
Date:	Mon, 5 Jan 2015 18:21:39 +0100
From:	Andreas Mohr <andi@...as.de>
To:	Joonsoo Kim <iamjoonsoo.kim@....com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Christoph Lameter <cl@...ux.com>,
	Pekka Enberg <penberg@...nel.org>,
	David Rientjes <rientjes@...gle.com>,
	Joonsoo Kim <iamjoonsoo.kim@....com>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org,
	Jesper Dangaard Brouer <brouer@...hat.com>
Subject: Re: [PATCH 6/6] mm/slab: allocation fastpath without disabling irq

Hi,

Joonsoo Kim wrote:
> + * Calculate the next globally unique transaction for disambiguiation

"disambiguation"

> +	ac->tid = next_tid(ac->tid);
(and all others)

object oriented:
array_cache_next_tid(ac);
(or perhaps rather: array_cache_start_transaction(ac);?).

> +	/*
> +	 * Because we disable irq just now, cpu can be changed
> +	 * and we are on different node with object node. In this rare
> +	 * case, just return pfmemalloc object for simplicity.
> +	 */

"are on a node which is different from object's node"

General thoughts (maybe just rambling, but that's just my feelings vs.
this mechanism, so maybe it's food for thought):
To me, the existing implementation seems too fond of IRQ fumbling
(i.e., affecting of oh so nicely *unrelated*
outer global environment context stuff).
A proper implementation wouldn't need *any* knowledge of this
(i.e., modifying such "IRQ disable" side effects,
to avoid having a scheduler hit and possibly ending up on another node).

Thus to me, the whole handling seems somewhat wrong and split
(since there remains the need to deal with scheduler distortion/disruption).
The bare-metal "inner" algorithm should not need to depend on such shenanigans
but simply be able to carry out its task unaffected,
where IRQs are simply always left enabled
(or at least potentially disabled by other kernel components only)
and the code then elegantly/inherently deals with IRQ complications.

Since the node change is scheduler-driven (I assume),
any (changes of) context attributes
which are relevant to (affect) SLAB-internal operations
ought to be implicitly/automatically re-assigned by the scheduler,
and then the most that should be needed is a *final* check in SLAB
(possibly in an outer user-facing layer of it)
whether the current final calculation result still matches expectations,
i.e. whether there was no disruption
(in which case we'd also do a goto redo: operation or some such :).

These thoughts also mean that I'm unsure (difficult to determine)
of whether this change is good (i.e. a clean step in the right direction),
or whether instead the implementation could easily directly be made
fully independent from IRQ constraints.

Thanks,

Andreas Mohr
--
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/