linux-kernel - Re: [PATCH 3/8] mm: introduce memalloc_nofs

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: <32751e40-51ed-bbeb-589c-6e0e2bd65de9@suse.cz>
Date:   Mon, 9 Jan 2017 15:04:11 +0100
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Michal Hocko <mhocko@...nel.org>
Cc:     linux-mm@...ck.org, linux-fsdevel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Dave Chinner <david@...morbit.com>, djwong@...nel.org,
        Theodore Ts'o <tytso@....edu>, Chris Mason <clm@...com>,
        David Sterba <dsterba@...e.cz>, Jan Kara <jack@...e.cz>,
        ceph-devel@...r.kernel.org, cluster-devel@...hat.com,
        linux-nfs@...r.kernel.org, logfs@...fs.org,
        linux-xfs@...r.kernel.org, linux-ext4@...r.kernel.org,
        linux-btrfs@...r.kernel.org, linux-mtd@...ts.infradead.org,
        reiserfs-devel@...r.kernel.org,
        linux-ntfs-dev@...ts.sourceforge.net,
        linux-f2fs-devel@...ts.sourceforge.net,
        linux-afs@...ts.infradead.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/8] mm: introduce memalloc_nofs_{save,restore} API

On 01/09/2017 02:42 PM, Michal Hocko wrote:
> On Mon 09-01-17 14:04:21, Vlastimil Babka wrote:
> [...]
>>> +static inline unsigned int memalloc_nofs_save(void)
>>> +{
>>> +	unsigned int flags = current->flags & PF_MEMALLOC_NOFS;
>>> +	current->flags |= PF_MEMALLOC_NOFS;
>>
>> So this is not new, as same goes for memalloc_noio_save, but I've
>> noticed that e.g. exit_signal() does tsk->flags |= PF_EXITING;
>> So is it possible that there's a r-m-w hazard here?
> 
> exit_signals operates on current and all task_struct::flags should be
> used only on the current.
> [...]

Ah, good to know.

> 
>>> @@ -3029,7 +3029,7 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg,
>>>  	int nid;
>>>  	struct scan_control sc = {
>>>  		.nr_to_reclaim = max(nr_pages, SWAP_CLUSTER_MAX),
>>> -		.gfp_mask = (gfp_mask & GFP_RECLAIM_MASK) |
>>> +		.gfp_mask = (current_gfp_context(gfp_mask) & GFP_RECLAIM_MASK) |
>>
>> So this function didn't do memalloc_noio_flags() before? Is it a bug
>> that should be fixed separately or at least mentioned? Because that
>> looks like a functional change...
> 
> We didn't need it. Kmem charges are opt-in and current all of them
> support GFP_IO. The LRU pages are not charged in NOIO context either.
> We need it now because there will be callers to charge GFP_KERNEL while
> being inside the NOFS scope.

I see.

> Now that you have opened this I have noticed that the code is wrong
> here because GFP_HIGHUSER_MOVABLE & ~GFP_RECLAIM_MASK would overwrite
> the removed GFP_FS. I guess it would be better and less error prone
> to move the current_gfp_context part into the direct reclaim entry -
> do_try_to_free_pages - and put the comment like this

Agree with your "So let's just scratch this follow up fix in the next
e-mail.

So for the unchanged patch.

Acked-by: Vlastimil Babka <vbabka@...e.cz>