Re: [patch 1/2]percpu_ida: fix a live lock

[Kent Overstreet <kmo@daterainc.com>]

On Sun, Jan 05, 2014 at 09:13:00PM +0800, Shaohua Li wrote:
> On Sat, Jan 04, 2014 at 01:08:04PM -0800, Kent Overstreet wrote:
> > On Tue, Dec 31, 2013 at 11:38:27AM +0800, Shaohua Li wrote:
> > > 
> > > steal_tags only happens when free tags is more than half of the total tags.
> > > This is too restrict and can cause live lock. I found one cpu has free tags,
> > > but other cpu can't steal (thread is bound to specific cpus), threads which
> > > wants to allocate tags are always sleeping. I found this when I run next patch,
> > > but this could happen without it I think.
> > > 
> > > I did performance test too with null_blk. Two cases (each cpu has enough percpu
> > > tags, or total tags are limited) haven't performance changes.
> > 
> > This doesn't appear to me to fix anything wrong with the current code
> > (and it'll hurt performance)
> 
> I suspects this hurts performance too, but it doesn't actually in my test.
> 
> > - we explicitly don't guarantee that all
> > the tags will be available for allocation at any given time, only half
> > of them. 
> 
> only half of the tags can be used? this is scaring. Of course we hope all tags
> are available.

No: that behaviour is explicitly documented and it's the behaviour we want.

There is an inherent performance tradeoff here between internal fragmentation
and performance: your patch tries to drive internal fragmentation to 0, but
under workloads that hit this, running on enough cores, this will have a VERY
high performance cost.

> > Can you explain more how this is being used where you're seeing
> > the issue? And I don't see the other patch in your patch series.
> 
> tasks are bound to specific cpus. And I saw one cpu has a lot of free (but less
> than half of total tags), but other cpus can't allocate tags, so I saw a live
> lock.

You're using it wrong - don't assume all nr_tags can be allocated. If you need
to guarantee that you'll always be able to allocate a certain number of tags,
you pass twice that to percpu_ida_init() instead.

Nacking this patch (and if you want me to look at it more you still to send me
the other patch in the series and show me your code that's using it).