linux-kernel - Re: [PATCH v2] mm/hugetlb: Defer freeing of huge pages if in non-task context

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: <20191217093143.GC31063@dhcp22.suse.cz>
Date:   Tue, 17 Dec 2019 10:31:43 +0100
From:   Michal Hocko <mhocko@...nel.org>
To:     Waiman Long <longman@...hat.com>
Cc:     Mike Kravetz <mike.kravetz@...cle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Matthew Wilcox <willy@...radead.org>,
        Davidlohr Bueso <dave@...olabs.net>,
        Andi Kleen <ak@...ux.intel.com>,
        "Aneesh Kumar K.V" <aneesh.kumar@...ux.ibm.com>
Subject: Re: [PATCH v2] mm/hugetlb: Defer freeing of huge pages if in
 non-task context

On Mon 16-12-19 20:25:08, Waiman Long wrote:
[...]
> Both the hugetbl_lock and the subpool lock can be acquired in
> free_huge_page(). One way to solve the problem is to make both locks
> irq-safe.

Please document why we do not take this, quite natural path and instead
we have to come up with an elaborate way instead. I believe the primary
motivation is that some operations under those locks are quite
expensive. Please add that to the changelog and ideally to the code as
well. We probably want to fix those anyway and then this would be a
temporary workaround.

> Another alternative is to defer the freeing to a workqueue job.
> 
> This patch implements the deferred freeing by adding a
> free_hpage_workfn() work function to do the actual freeing. The
> free_huge_page() call in a non-task context saves the page to be freed
> in the hpage_freelist linked list in a lockless manner.

Do we need to over complicate this (presumably) rare event by a lockless
algorithm? Why cannot we use a dedicated spin lock for for the linked
list manipulation? This should be really a trivial code without an
additional burden of all the lockless subtleties.

> +	pr_debug("HugeTLB: free_hpage_workfn() frees %d huge page(s)\n", cnt);

Why do we need the debugging message here?
-- 
Michal Hocko
SUSE Labs