LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jens Axboe <axboe@kernel.dk>
To: Matthew Wilcox <willy@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
	linux-kernel@vger.kernel.org, tglx@linutronix.de,
	Nicholas Bellinger <nab@linux-iscsi.org>,
	Shaohua Li <shli@fb.com>,
	Kent Overstreet <kent.overstreet@gmail.com>
Subject: Re: [PATCH] percpu_ida: Use _irqsave() instead of local_irq_save() + spin_lock
Date: Mon, 7 May 2018 15:34:37 -0600	[thread overview]
Message-ID: <c5151b40-9f2a-fa64-2652-01b109e85954@kernel.dk> (raw)
In-Reply-To: <20180507134731.GA28974@bombadil.infradead.org>

On 5/7/18 7:47 AM, Matthew Wilcox wrote:
> On Sat, May 05, 2018 at 08:52:02AM -0700, Matthew Wilcox wrote:
>> init and destroy seem to map to sbitmap_queue_init_node and
>> sbitmap_queue_free.  percpu_ida_free maps to sbitmap_queue_clear.
> 
> Hmm.
> 
> void sbitmap_queue_clear(struct sbitmap_queue *sbq, unsigned int nr,
>                          unsigned int cpu)
> {
>         sbitmap_clear_bit_unlock(&sbq->sb, nr);
>         sbq_wake_up(sbq);
>         if (likely(!sbq->round_robin && nr < sbq->sb.depth))
>                 *per_cpu_ptr(sbq->alloc_hint, cpu) = nr;
> }
> EXPORT_SYMBOL_GPL(sbitmap_queue_clear);
> 
> If we free a tag on a CPU other than the one it's allocated on, that seems
> like it's going to guarantee a cacheline pingpong.  Is the alloc_hint
> really that valuable?  I'd be tempted to maintain the alloc_hint (if it's
> at all valuable) as being just a hint for which word to look at first,
> and only update it on allocation, rather than updating it on free.

The point is to update it on free, so we know where to start the search.
Ideally it'd still be free. Keep in mind that some of this was driven
by blk-mq developments, on how to retain fast allocations without
adding per-cpu caches (which we can't afford, see previous rant on
tag space utilization). On blk-mq, the free is generally on the CPU
you allocated, or close to in terms of caching. Even for shared tag
cases, I haven't seen any bad behavior from bouncing. Doesn't mean
that we could not, but I'd be wary of changing any of it without
substantial performance testing.

> Then we can drop the 'cpu' argument to sbitmap_queue_clear(), which
> would help this conversion because the percpu_ida users don't know what
> CPU their tag was allocated on.

What you want is something around the sbitmap_queue_{get,clear} that
wraps the alloc hint and wait queue mechanism, so the caller doesn't
have to deal with it. We don't have that, since the blk-mq mechanism
works out better just implementing it. We have things like queue
runs that are don't for the failure case.

-- 
Jens Axboe

      reply	other threads:[~2018-05-07 21:34 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-04 15:32 Sebastian Andrzej Siewior
2018-05-04 23:22 ` Andrew Morton
2018-05-05  3:51   ` Matthew Wilcox
2018-05-05 14:10     ` Jens Axboe
2018-05-05 14:42       ` Jens Axboe
2018-05-05 15:52       ` Matthew Wilcox
2018-05-07 13:47         ` Matthew Wilcox
2018-05-07 21:34           ` Jens Axboe [this message]

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=c5151b40-9f2a-fa64-2652-01b109e85954@kernel.dk \
    --to=axboe@kernel.dk \
    --cc=akpm@linux-foundation.org \
    --cc=bigeasy@linutronix.de \
    --cc=kent.overstreet@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nab@linux-iscsi.org \
    --cc=shli@fb.com \
    --cc=tglx@linutronix.de \
    --cc=willy@infradead.org \
    --subject='Re: [PATCH] percpu_ida: Use _irqsave() instead of local_irq_save() + spin_lock' \
    /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).