LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Vlastimil Babka <vbabka@suse.cz>
To: Mike Galbraith <efault@gmx.de>, Clark Williams <williams@redhat.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
	Thomas Gleixner <tglx@linutronix.de>,
	LKML <linux-kernel@vger.kernel.org>,
	RT <linux-rt-users@vger.kernel.org>
Subject: Re: [ANNOUNCE] v5.14-rc5-rt8
Date: Sun, 15 Aug 2021 11:35:53 +0200	[thread overview]
Message-ID: <c92fc2cb-03cd-d6a2-fb4a-7bc33e94e391@suse.cz> (raw)
In-Reply-To: <5dbf6cf9e82ef15ce0febf070608da2d5b128763.camel@gmx.de>

On 8/15/21 6:17 AM, Mike Galbraith wrote:
> On Sat, 2021-08-14 at 21:08 +0200, Vlastimil Babka wrote:
>>
>> Aha! That's helpful. Hopefully it's just a small issue where we
>> opportunistically test flags on a page that's protected by the local
>> lock we didn't take yet, and I didn't realize there's the VM_BUG_ON
>> which can trigger if our page went away (which we would have realized
>> after taking the lock).
> 
> Speaking of optimistic peeking perhaps going badly, why is the below
> not true?  There's protection against ->partial going disappearing
> during a preemption... but can't it just as easily appear, so where is
> that protection?

If it appears, it appears, we don't care, we just leave it there and
won't use it.

> If the other side of that window is safe, it could use a comment so
> dummies reading this code don't end up asking mm folks why the heck
> they don't just take the darn lock and be done with it instead of tap
> dancing all around thething :)

Well, with your patch, ->partial might appear just after the unlock, so
does that really change anything?
The point is to avoid the taking the lock if it's almost certain there
will be nothing to gain.

c->partial appearing is easy to just ignore. c->page appearing, while we
got our own page, is worse as there can be only one c->page. But it's
unavoidable, we can't just keep holding the local lock while going to
the page allocator etc. That's why under retry_load_page: we have to
deactivate a c->page that appeared under us...

> ---
>  mm/slub.c |   14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2937,17 +2937,16 @@ static void *___slab_alloc(struct kmem_c
> 
>  new_slab:
> 
> +	/*
> +	 * To avoid false negative race with put_cpu_partial() during a
> +	 * preemption, we must call slub_percpu_partial() under lock.
> +	 */
> +	local_lock_irqsave(&s->cpu_slab->lock, flags);
>  	if (slub_percpu_partial(c)) {
> -		local_lock_irqsave(&s->cpu_slab->lock, flags);
>  		if (unlikely(c->page)) {
>  			local_unlock_irqrestore(&s->cpu_slab->lock, flags);
>  			goto reread_page;
>  		}
> -		if (unlikely(!slub_percpu_partial(c))) {
> -			local_unlock_irqrestore(&s->cpu_slab->lock, flags);
> -			/* we were preempted and partial list got empty */
> -			goto new_objects;
> -		}
> 
>  		page = c->page = slub_percpu_partial(c);
>  		slub_set_percpu_partial(c, page);
> @@ -2955,8 +2954,7 @@ static void *___slab_alloc(struct kmem_c
>  		stat(s, CPU_PARTIAL_ALLOC);
>  		goto redo;
>  	}
> -
> -new_objects:
> +	local_unlock_irqrestore(&s->cpu_slab->lock, flags);
> 
>  	freelist = get_partial(s, gfpflags, node, &page);
>  	if (freelist)
> 
> 


  reply	other threads:[~2021-08-15  9:36 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-10 16:37 Sebastian Andrzej Siewior
2021-08-12 20:18 ` Clark Williams
2021-08-12 20:45   ` Vlastimil Babka
2021-08-12 21:24     ` Clark Williams
2021-08-12 21:30       ` Vlastimil Babka
2021-08-12 21:44         ` Clark Williams
2021-08-14 19:08           ` Vlastimil Babka
2021-08-15  3:13             ` Mike Galbraith
2021-08-15  9:01               ` Mike Galbraith
2021-08-15  4:17             ` Mike Galbraith
2021-08-15  9:35               ` Vlastimil Babka [this message]
2021-08-15 13:03                 ` Mike Galbraith
2021-08-12 21:36   ` Vlastimil Babka
2021-08-12 21:47     ` Clark Williams
2021-08-13 10:56   ` Mike Galbraith
2021-08-14  5:33     ` Mike Galbraith
2021-08-14 18:28       ` Vlastimil Babka
2021-08-15  1:26         ` Mike Galbraith

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c92fc2cb-03cd-d6a2-fb4a-7bc33e94e391@suse.cz \
    --to=vbabka@suse.cz \
    --cc=bigeasy@linutronix.de \
    --cc=efault@gmx.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rt-users@vger.kernel.org \
    --cc=tglx@linutronix.de \
    --cc=williams@redhat.com \
    --subject='Re: [ANNOUNCE] v5.14-rc5-rt8' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).