Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1680620 > unrolled thread

Re: [GIT pull] irq updates for 4.13

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [GIT pull] irq updates for 4.13 Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-04 02:10 +0200
    Re: [GIT pull] irq updates for 4.13 Thomas Gleixner <tglx@linutronix.de> - 2017-07-04 10:20 +0200
      Re: [GIT pull] irq updates for 4.13 Thomas Gleixner <tglx@linutronix.de> - 2017-07-04 12:30 +0200
    Re: [GIT pull] irq updates for 4.13 Jens Axboe <axboe@kernel.dk> - 2017-07-04 17:20 +0200
      Re: [GIT pull] irq updates for 4.13 Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-04 20:40 +0200
        Re: [GIT pull] irq updates for 4.13 Thomas Gleixner <tglx@linutronix.de> - 2017-07-04 21:20 +0200
        Re: [GIT pull] irq updates for 4.13 Jens Axboe <axboe@kernel.dk> - 2017-07-05 00:00 +0200
    Re: [GIT pull] irq updates for 4.13 Christoph Hellwig <hch@lst.de> - 2017-07-05 17:20 +0200

#1680620 — Re: [GIT pull] irq updates for 4.13

On Mon, Jul 3, 2017 at 12:42 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> please pull the latest irq-core-for-linus git tree from:
>
>    git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq-core-for-linus

Ugh, this caused conflicts with the block tree, with commits

 - fe631457ff3e: "blk-mq: map all HWQ also in hyperthreaded system"

 - 5f042e7cbd9e "blk-mq: Include all present CPUs in the default queue mapping"

clashing.

I'm not at all understanding why that second commit came in through
the irq tree at all, in fact. Very annoying. Why was that not sent
through the block tree? It doesn't seem to have anything fundamentally
to do with irqs, really: it's a driver CPU choice for irq chocie.

Anyway, I absolutely detested that code, and the obvious resolution
was too disgusting to live. So I did an evil merge and moved some
things around in the merge to make it at least not cause me to dig my
eyes out.

But I'd like people to look at that - not so much due to the evil
merge itself (but check that too, by any means), but just because the
code seems fundamentally broken for the hotplug case. We end up
picking a possible metric shit-ton of CPU's for queue 0, if they were
"possible but not online".

If they ever do come online, does that get fixed? I don't know.
Somebody should check.

                    Linus

[toc] | [next] | [standalone]

#1680748

On Mon, 3 Jul 2017, Linus Torvalds wrote:

> On Mon, Jul 3, 2017 at 12:42 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > please pull the latest irq-core-for-linus git tree from:
> >
> >    git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq-core-for-linus
> 
> Ugh, this caused conflicts with the block tree, with commits
> 
>  - fe631457ff3e: "blk-mq: map all HWQ also in hyperthreaded system"
> 
>  - 5f042e7cbd9e "blk-mq: Include all present CPUs in the default queue mapping"
> 
> clashing.
> 
> I'm not at all understanding why that second commit came in through
> the irq tree at all, in fact. Very annoying. Why was that not sent
> through the block tree? It doesn't seem to have anything fundamentally
> to do with irqs, really: it's a driver CPU choice for irq chocie.

There is a dependency. The changes in the block code rely on the new
features of the generic interrupt affinity management. See below.

> Anyway, I absolutely detested that code, and the obvious resolution
> was too disgusting to live. So I did an evil merge and moved some
> things around in the merge to make it at least not cause me to dig my
> eyes out.
> 
> But I'd like people to look at that - not so much due to the evil
> merge itself (but check that too, by any means), but just because the
> code seems fundamentally broken for the hotplug case. We end up
> picking a possible metric shit-ton of CPU's for queue 0, if they were
> "possible but not online".

The mechanism is:

Spread out the queues and the associated interrupts accross the possible
CPUs. This results in a queue/interrupt per group of CPUs (group can be a
single CPU)

If a group is offline, then the interrupt is kept in managed shutdown
mode. If a CPU of the group comes online then the core management starts up
the interrupt and makes it affine to that CPU.

If the last CPU of a group goes offline, the interrupt is not moved to some
random other CPU. It's put in managed shutdown mode and then restarted when
the a CPU of the group comes online again.

That exercise avoids exactly the 'metric tons of irqs' moved to random CPUs
and then brought back to the target CPUs when they come online
again. On/offline seems to be (ab)used frequently for power management
purposes nowadays.

Sorry, if I did not make that clear enough in the pull request message.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1680844

On Tue, 4 Jul 2017, Thomas Gleixner wrote:
> On Mon, 3 Jul 2017, Linus Torvalds wrote:
> 
> > On Mon, Jul 3, 2017 at 12:42 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > >
> > > please pull the latest irq-core-for-linus git tree from:
> > >
> > >    git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq-core-for-linus
> > 
> > Ugh, this caused conflicts with the block tree, with commits
> > 
> >  - fe631457ff3e: "blk-mq: map all HWQ also in hyperthreaded system"
> > 
> >  - 5f042e7cbd9e "blk-mq: Include all present CPUs in the default queue mapping"
> > 
> > clashing.
> > 
> > I'm not at all understanding why that second commit came in through
> > the irq tree at all, in fact. Very annoying. Why was that not sent
> > through the block tree? It doesn't seem to have anything fundamentally
> > to do with irqs, really: it's a driver CPU choice for irq chocie.
> 
> There is a dependency. The changes in the block code rely on the new
> features of the generic interrupt affinity management. See below.

That said, we should have kept the colliding changes back, wait until
block and irq got merged and then apply them again properly.

Sorry for the inconveniance.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1681042

On 07/03/2017 06:00 PM, Linus Torvalds wrote:
> But I'd like people to look at that - not so much due to the evil
> merge itself (but check that too, by any means), but just because the
> code seems fundamentally broken for the hotplug case. We end up
> picking a possible metric shit-ton of CPU's for queue 0, if they were
> "possible but not online".
> 
> If they ever do come online, does that get fixed? I don't know.
> Somebody should check.

Yes, the blk-mq cpu hotplug code updates mappings when CPUs come and
go, so that part is fine. That's exercised everytime the laptop is
suspended and resumed.

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

#1681122

On Tue, Jul 4, 2017 at 8:17 AM, Jens Axboe <axboe@kernel.dk> wrote:
> On 07/03/2017 06:00 PM, Linus Torvalds wrote:
>>
>> If they ever do come online, does that get fixed? I don't know.
>> Somebody should check.
>
> Yes, the blk-mq cpu hotplug code updates mappings when CPUs come and
> go, so that part is fine. That's exercised everytime the laptop is
> suspended and resumed.

I don't think that's true any more. Commit fe631457ff3e changed it to
map the initial CPU's sequentially whether they are online or not.
Only after you run out of hardware queues will we start playing games.

That's what worries me about the conflict - the two changes did very
different things to the same code. I'd really like somebody to take a
look at my resolution, and just in general how those two different
changes work together.

                 Linus

[toc] | [prev] | [next] | [standalone]

#1681137

On Tue, 4 Jul 2017, Linus Torvalds wrote:
> On Tue, Jul 4, 2017 at 8:17 AM, Jens Axboe <axboe@kernel.dk> wrote:
> > On 07/03/2017 06:00 PM, Linus Torvalds wrote:
> >>
> >> If they ever do come online, does that get fixed? I don't know.
> >> Somebody should check.
> >
> > Yes, the blk-mq cpu hotplug code updates mappings when CPUs come and
> > go, so that part is fine. That's exercised everytime the laptop is
> > suspended and resumed.
> 
> I don't think that's true any more. Commit fe631457ff3e changed it to
> map the initial CPU's sequentially whether they are online or not.
> Only after you run out of hardware queues will we start playing games.
> 
> That's what worries me about the conflict - the two changes did very
> different things to the same code. I'd really like somebody to take a
> look at my resolution, and just in general how those two different
> changes work together.

Hmm, I leave that to Christoph. He wrote the irq stuff and reviewed
fe631457ff3e.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1681199

On 07/04/2017 12:34 PM, Linus Torvalds wrote:
> On Tue, Jul 4, 2017 at 8:17 AM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 07/03/2017 06:00 PM, Linus Torvalds wrote:
>>>
>>> If they ever do come online, does that get fixed? I don't know.
>>> Somebody should check.
>>
>> Yes, the blk-mq cpu hotplug code updates mappings when CPUs come and
>> go, so that part is fine. That's exercised everytime the laptop is
>> suspended and resumed.
> 
> I don't think that's true any more. Commit fe631457ff3e changed it to
> map the initial CPU's sequentially whether they are online or not.
> Only after you run out of hardware queues will we start playing games.
> 
> That's what worries me about the conflict - the two changes did very
> different things to the same code. I'd really like somebody to take a
> look at my resolution, and just in general how those two different
> changes work together.

OK, I'll take a look at it tomorrow.

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

#1681576

On Mon, Jul 03, 2017 at 05:00:03PM -0700, Linus Torvalds wrote:
> I'm not at all understanding why that second commit came in through
> the irq tree at all, in fact. Very annoying. Why was that not sent
> through the block tree? It doesn't seem to have anything fundamentally
> to do with irqs, really: it's a driver CPU choice for irq chocie.

It depends on a major IRQ layer rework, and it had at that time no
clash with the block tree at all.  That's why I suggested to Thomas
and Jens that we should take it through the irq code.  Then Max'
code showed up last minute and wrecked that plan, and no one noticed
in time.  Blame it on me for not noticing this in time.

> Anyway, I absolutely detested that code, and the obvious resolution
> was too disgusting to live. So I did an evil merge and moved some
> things around in the merge to make it at least not cause me to dig my
> eyes out.
> 
> But I'd like people to look at that - not so much due to the evil
> merge itself (but check that too, by any means), but just because the
> code seems fundamentally broken for the hotplug case. We end up
> picking a possible metric shit-ton of CPU's for queue 0, if they were
> "possible but not online".

I think this code also needs to move over to cpu_possible instead
of cpu_online to match what we did for the MSI-X based mapping.  But
I'll need a little more coffee and get back into it.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web