linux-kernel - Re: [PATCH v3 1/3] mm/vmalloc: remove preempt

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20191018093741.GA8744@pc636>
Date:   Fri, 18 Oct 2019 11:37:41 +0200
From:   Uladzislau Rezki <urezki@...il.com>
To:     Michal Hocko <mhocko@...nel.org>
Cc:     "Uladzislau Rezki (Sony)" <urezki@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Daniel Wagner <dwagner@...e.de>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Thomas Gleixner <tglx@...utronix.de>, linux-mm@...ck.org,
        LKML <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Hillf Danton <hdanton@...a.com>,
        Matthew Wilcox <willy@...radead.org>,
        Oleksiy Avramchenko <oleksiy.avramchenko@...ymobile.com>,
        Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH v3 1/3] mm/vmalloc: remove preempt_disable/enable when do
 preloading

Hello, Michal.

Sorry for late reply. See my comments enclosed below:

> On Wed 16-10-19 11:54:36, Uladzislau Rezki (Sony) wrote:
> > Some background. The preemption was disabled before to guarantee
> > that a preloaded object is available for a CPU, it was stored for.
> 
> Probably good to be explicit that this has been achieved by combining
> the disabling the preemption and taking the spin lock while the
> ne_fit_preload_node is checked resp. repopulated, right?
> 
Right, agree with your comment!

> > The aim was to not allocate in atomic context when spinlock
> > is taken later, for regular vmap allocations. But that approach
> > conflicts with CONFIG_PREEMPT_RT philosophy. It means that
> > calling spin_lock() with disabled preemption is forbidden
> > in the CONFIG_PREEMPT_RT kernel.
> > 
> > Therefore, get rid of preempt_disable() and preempt_enable() when
> > the preload is done for splitting purpose. As a result we do not
> > guarantee now that a CPU is preloaded, instead we minimize the
> > case when it is not, with this change.
> 
> by populating the per cpu preload pointer under the vmap_area_lock.
> This implies that at least each caller which has done the preallocation
> will not fallback to an atomic allocation later. It is possible that the
> preallocation would be pointless or that no preallocation is done
> because of the race but your data shows that this is really rare.
> 
That makes sense to add. Please find below updated comment:

<snip>
mm/vmalloc: remove preempt_disable/enable when do preloading

Some background. The preemption was disabled before to guarantee
that a preloaded object is available for a CPU, it was stored for.
That was achieved by combining the disabling the preemption and
taking the spin lock while the ne_fit_preload_node is checked.

The aim was to not allocate in atomic context when spinlock
is taken later, for regular vmap allocations. But that approach
conflicts with CONFIG_PREEMPT_RT philosophy. It means that
calling spin_lock() with disabled preemption is forbidden
in the CONFIG_PREEMPT_RT kernel.

Therefore, get rid of preempt_disable() and preempt_enable() when
the preload is done for splitting purpose. As a result we do not
guarantee now that a CPU is preloaded, instead we minimize the
case when it is not, with this change, by populating the per
cpu preload pointer under the vmap_area_lock.

This implies that at least each caller that has done the preallocation
will not fallback to an atomic allocation later. It is possible
that the preallocation would be pointless or that no preallocation
is done because of the race but the data shows that this is really
rare.

For example i run the special test case that follows the preload
pattern and path. 20 "unbind" threads run it and each does
1000000 allocations. Only 3.5 times among 1000000 a CPU was
not preloaded. So it can happen but the number is negligible.

V2 - > V3:
    - update the commit message

V1 -> V2:
  - move __this_cpu_cmpxchg check when spin_lock is taken,
    as proposed by Andrew Morton
  - add more explanation in regard of preloading
  - adjust and move some comments
<snip>

Do you agree on that?

Thank you!

--
Vlad Rezki