The Linux Kernel Mailing List
 help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: Keith Busch <keith.busch@intel.com>
Cc: linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org,
	Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH] irq/affinity: Assign all CPUs a vector
Date: Fri, 31 Mar 2017 12:59:12 +0200 (CEST)	[thread overview]
Message-ID: <alpine.DEB.2.20.1703311247160.1780@nanos> (raw)
In-Reply-To: <1490743277-14139-1-git-send-email-keith.busch@intel.com>

On Tue, 28 Mar 2017, Keith Busch wrote:

> The number of vectors to assign needs to be adjusted for each node such
> that it doesn't exceed the number of CPUs in that node. This patch
> recalculates the vector assignment per-node so that we don't try to
> assign more vectors than there are CPUs. When that previously happened,
> the cpus_per_vec was calculated to be 0, so many vectors had no CPUs
> assigned. This then goes on to fail to allocate descriptors due to
> empty masks, leading to an unoptimal spread.

To be honest: This changelog sucks. I really have a hard time to figure out
what's wrong. Can you please structure and rephrase this so it's
understandable for people who did not debug the issue five days ago? That
includes yourself when you have to look at that patch 3 month from now.

A proper structure would be: Context - Problem - Solution. e.g.

  irq_create_affinity_masks() spreads the interrupt vectors of a multi
  queue device across CPUs.

  The algorithm fails to do X, which causes problem Y

  Make it do frotz so the blas are properly assigned.

> Not only does this patch get the intended spread, this also fixes
> other subsystems that depend on every CPU being assigned to something:
> blk_mq_map_swqueue dereferences NULL while mapping s/w queues when CPUs
> are unnassigned, so making sure all CPUs are assigned fixes that.

That's hardly a justification for that change. If blk_mq_map_swqueue()
lacks sanity checks, then blk_mq_map_swqueue() is broken and needs to be
fixed independently of this.

Thanks,

	tglx

      parent reply	other threads:[~2017-03-31 10:59 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-28 23:21 [PATCH] irq/affinity: Assign all CPUs a vector Keith Busch
2017-03-29 17:15 ` Sagi Grimberg
2017-03-29 17:54   ` Keith Busch
2017-03-29 17:50     ` Sagi Grimberg
2017-03-30  8:21 ` Christoph Hellwig
2017-03-30 17:12   ` Keith Busch
2017-04-12  1:33     ` [lkp-robot] [irq/affinity] 13c024422c: fsmark.files_per_sec -4.3% regression kernel test robot
2017-04-12 15:02       ` Keith Busch
2017-03-31 10:59 ` Thomas Gleixner [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=alpine.DEB.2.20.1703311247160.1780@nanos \
    --to=tglx@linutronix.de \
    --cc=hch@lst.de \
    --cc=keith.busch@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nvme@lists.infradead.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox