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

Groups > linux.kernel > #1436254 > unrolled thread

Re: [PATCH 11/13] blk-mq: allow the driver to pass in an affinity mask

Started byAlexander Gordeev <agordeev@redhat.com>
First post2016-07-04 10:20 +0200
Last post2016-07-12 08:40 +0200
Articles 5 — 2 participants

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: [PATCH 11/13] blk-mq: allow the driver to pass in an affinity  mask Alexander Gordeev <agordeev@redhat.com> - 2016-07-04 10:20 +0200
    Re: [PATCH 11/13] blk-mq: allow the driver to pass in an affinity  mask Christoph Hellwig <hch@lst.de> - 2016-07-04 10:40 +0200
      Re: [PATCH 11/13] blk-mq: allow the driver to pass in an affinity  mask Alexander Gordeev <agordeev@redhat.com> - 2016-07-04 11:40 +0200
        Re: [PATCH 11/13] blk-mq: allow the driver to pass in an affinity  mask Christoph Hellwig <hch@lst.de> - 2016-07-10 05:50 +0200
          Re: [PATCH 11/13] blk-mq: allow the driver to pass in an affinity  mask Alexander Gordeev <agordeev@redhat.com> - 2016-07-12 08:40 +0200

#1436254 — Re: [PATCH 11/13] blk-mq: allow the driver to pass in an affinity mask

FromAlexander Gordeev <agordeev@redhat.com>
Date2016-07-04 10:20 +0200
SubjectRe: [PATCH 11/13] blk-mq: allow the driver to pass in an affinity mask
Message-ID<rR7gJ-3eC-3@gated-at.bofh.it>
On Tue, Jun 14, 2016 at 09:59:04PM +0200, Christoph Hellwig wrote:
> +static int blk_mq_create_mq_map(struct blk_mq_tag_set *set,
> +		const struct cpumask *affinity_mask)
> +{
> +	int queue = -1, cpu = 0;
> +
> +	set->mq_map = kzalloc_node(sizeof(*set->mq_map) * nr_cpu_ids,
> +			GFP_KERNEL, set->numa_node);
> +	if (!set->mq_map)
> +		return -ENOMEM;
> +
> +	if (!affinity_mask)
> +		return 0;	/* map all cpus to queue 0 */
> +
> +	/* If cpus are offline, map them to first hctx */
> +	for_each_online_cpu(cpu) {
> +		if (cpumask_test_cpu(cpu, affinity_mask))
> +			queue++;

CPUs missing in an affinity mask are mapped to hctxs. Is that intended?

> +		if (queue > 0)

Why this check?

> +			set->mq_map[cpu] = queue;
> +	}
> +
> +	return 0;
> +}
> +

[toc] | [next] | [standalone]

#1436299

FromChristoph Hellwig <hch@lst.de>
Date2016-07-04 10:40 +0200
Message-ID<rR7A5-3l0-7@gated-at.bofh.it>
In reply to#1436254
On Mon, Jul 04, 2016 at 10:15:41AM +0200, Alexander Gordeev wrote:
> On Tue, Jun 14, 2016 at 09:59:04PM +0200, Christoph Hellwig wrote:
> > +static int blk_mq_create_mq_map(struct blk_mq_tag_set *set,
> > +		const struct cpumask *affinity_mask)
> > +{
> > +	int queue = -1, cpu = 0;
> > +
> > +	set->mq_map = kzalloc_node(sizeof(*set->mq_map) * nr_cpu_ids,
> > +			GFP_KERNEL, set->numa_node);
> > +	if (!set->mq_map)
> > +		return -ENOMEM;
> > +
> > +	if (!affinity_mask)
> > +		return 0;	/* map all cpus to queue 0 */
> > +
> > +	/* If cpus are offline, map them to first hctx */
> > +	for_each_online_cpu(cpu) {
> > +		if (cpumask_test_cpu(cpu, affinity_mask))
> > +			queue++;
> 
> CPUs missing in an affinity mask are mapped to hctxs. Is that intended?

Yes - each CPU needs to be mapped to some hctx, otherwise we can't
submit I/O from that CPU.

> > +		if (queue > 0)
> 
> Why this check?
> 
> > +			set->mq_map[cpu] = queue;

mq_map is initialized to zero already, so we don't really need the
assignment for queue 0.  The reason why this check exists is because
we start with queue = -1 and we never want to assignment -1 to mq_map.

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

#1436396

FromAlexander Gordeev <agordeev@redhat.com>
Date2016-07-04 11:40 +0200
Message-ID<rR8wa-3U6-5@gated-at.bofh.it>
In reply to#1436299
On Mon, Jul 04, 2016 at 10:38:49AM +0200, Christoph Hellwig wrote:
> On Mon, Jul 04, 2016 at 10:15:41AM +0200, Alexander Gordeev wrote:
> > On Tue, Jun 14, 2016 at 09:59:04PM +0200, Christoph Hellwig wrote:
> > > +static int blk_mq_create_mq_map(struct blk_mq_tag_set *set,
> > > +		const struct cpumask *affinity_mask)
> > > +{
> > > +	int queue = -1, cpu = 0;
> > > +
> > > +	set->mq_map = kzalloc_node(sizeof(*set->mq_map) * nr_cpu_ids,
> > > +			GFP_KERNEL, set->numa_node);
> > > +	if (!set->mq_map)
> > > +		return -ENOMEM;
> > > +
> > > +	if (!affinity_mask)
> > > +		return 0;	/* map all cpus to queue 0 */
> > > +
> > > +	/* If cpus are offline, map them to first hctx */
> > > +	for_each_online_cpu(cpu) {
> > > +		if (cpumask_test_cpu(cpu, affinity_mask))
> > > +			queue++;
> > 
> > CPUs missing in an affinity mask are mapped to hctxs. Is that intended?
> 
> Yes - each CPU needs to be mapped to some hctx, otherwise we can't
> submit I/O from that CPU.
> 
> > > +		if (queue > 0)
> > 
> > Why this check?
> > 
> > > +			set->mq_map[cpu] = queue;
> 
> mq_map is initialized to zero already, so we don't really need the
> assignment for queue 0.  The reason why this check exists is because
> we start with queue = -1 and we never want to assignment -1 to mq_map.

Would this read better then?

	int queue = 0;

	...

	/* If cpus are offline, map them to first hctx */
	for_each_online_cpu(cpu) {
		set->mq_map[cpu] = queue;
		if (cpumask_test_cpu(cpu, affinity_mask))
			queue++;
	}

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

#1440025

FromChristoph Hellwig <hch@lst.de>
Date2016-07-10 05:50 +0200
Message-ID<rTdUJ-3GE-1@gated-at.bofh.it>
In reply to#1436396
On Mon, Jul 04, 2016 at 11:35:28AM +0200, Alexander Gordeev wrote:
> > mq_map is initialized to zero already, so we don't really need the
> > assignment for queue 0.  The reason why this check exists is because
> > we start with queue = -1 and we never want to assignment -1 to mq_map.
> 
> Would this read better then?
> 
> 	int queue = 0;
> 
> 	...
> 
> 	/* If cpus are offline, map them to first hctx */
> 	for_each_online_cpu(cpu) {
> 		set->mq_map[cpu] = queue;
> 		if (cpumask_test_cpu(cpu, affinity_mask))
> 			queue++;

It would read better, but I don't think it's actually correct.
We'd still assign the 'old' queue to the cpu that is set in the affinity
mask.

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

#1441084

FromAlexander Gordeev <agordeev@redhat.com>
Date2016-07-12 08:40 +0200
Message-ID<rTZwm-1fs-5@gated-at.bofh.it>
In reply to#1440025
On Sun, Jul 10, 2016 at 05:41:44AM +0200, Christoph Hellwig wrote:
> On Mon, Jul 04, 2016 at 11:35:28AM +0200, Alexander Gordeev wrote:
> > > mq_map is initialized to zero already, so we don't really need the
> > > assignment for queue 0.  The reason why this check exists is because
> > > we start with queue = -1 and we never want to assignment -1 to mq_map.
> > 
> > Would this read better then?
> > 
> > 	int queue = 0;
> > 
> > 	...
> > 
> > 	/* If cpus are offline, map them to first hctx */
> > 	for_each_online_cpu(cpu) {
> > 		set->mq_map[cpu] = queue;
> > 		if (cpumask_test_cpu(cpu, affinity_mask))
> > 			queue++;
> 
> It would read better, but I don't think it's actually correct.
> We'd still assign the 'old' queue to the cpu that is set in the affinity
> mask.

To be honest, I fail to see a functional difference, but it is just
a nit anyway.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web