linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: Bart Van Assche <bvanassche@acm.org>, Jens Axboe <axboe@kernel.dk>
Cc: "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	Christoph Hellwig <hch@lst.de>, Jaegeuk Kim <jaegeuk@kernel.org>,
	Hannes Reinecke <hare@suse.de>, Ming Lei <ming.lei@redhat.com>,
	Johannes Thumshirn <Johannes.Thumshirn@wdc.com>,
	Himanshu Madhani <himanshu.madhani@oracle.com>
Subject: Re: [PATCH 04/14] block: Introduce the ioprio rq-qos policy
Date: Thu, 10 Jun 2021 00:29:17 +0000	[thread overview]
Message-ID: <DM6PR04MB7081960D31CBB02438C29D27E7359@DM6PR04MB7081.namprd04.prod.outlook.com> (raw)
In-Reply-To: <98dbb3dc-d925-f2a8-7f34-94f9c56bb257@acm.org>

On 2021/06/10 1:49, Bart Van Assche wrote:
> On 6/8/21 9:40 PM, Damien Le Moal wrote:
>> On 2021/06/09 8:07, Bart Van Assche wrote:
> 
> [ ... ]
> 
>>> +/*
>>> + * The accepted I/O priority values are 0..IOPRIO_CLASS_MAX(3). 0 (default)
>>> + * means do not modify the I/O priority. Values 1..3 are used to restrict the
>>> + * I/O priority for a cgroup to the specified priority. See also
>>> + * <linux/ioprio.h>.
>>> + */
>>> +#define IOPRIO_CLASS_MAX IOPRIO_CLASS_IDLE
> 
> [ ... ]
> 
>>> +static int ioprio_set_prio_class(struct cgroup_subsys_state *css,
>>> +				 struct cftype *cft, u64 val)
>>> +{
>>> +	struct ioprio_blkcg *blkcg = ioprio_blkcg_from_css(css);
>>> +
>>> +	if (val > IOPRIO_CLASS_MAX)
>>> +		return -EINVAL;
>>
>> Where is IOPRIO_CLASS_MAX defined ? I do not see it.
> 
> Near the start of the new block/blk-ioprio.c file.

OK. I missed that. Shouldn't this definition be moved to include/linux/ioprio.h
? It can be added as the last entry of the class enum.

> 
>> Why not use ioprio_valid() ?
> 
> The definition of that macro is as follows:
> 
> #define ioprio_valid(mask)	\
> 	(IOPRIO_PRIO_CLASS((mask)) != IOPRIO_CLASS_NONE)
> 
> So that macro accepts I/O priority classes 1..7 while the above code
> accepts I/O priority classes 0..3. I think that ioprio_set_prio_class()
> should only accept I/O priority class values 0..3.

Yes. I misread this macro definition. The name is actually confusing. It does
not check that the ioprio is valid, it only check that it is not NONE, meaning
that something is set, but that something may not be valid. We could probably
turn this macro into an inline function that does a better job at checking that
valid values are in the "mask" argument, which by the way is also badly named,
since that is not a mask at all, but an ioprio.



-- 
Damien Le Moal
Western Digital Research

  reply	other threads:[~2021-06-10  0:29 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-08 23:06 [PATCH 00/14] Improve I/O priority support Bart Van Assche
2021-06-08 23:06 ` [PATCH 01/14] block/Kconfig: Make the BLK_WBT and BLK_WBT_MQ entries consecutive Bart Van Assche
2021-06-09  4:14   ` Damien Le Moal
2021-06-09  7:03   ` Johannes Thumshirn
2021-06-10  6:02   ` Hannes Reinecke
2021-06-08 23:06 ` [PATCH 02/14] block/blk-cgroup: Swap the blk_throtl_init() and blk_iolatency_init() calls Bart Van Assche
2021-06-09  4:19   ` Damien Le Moal
2021-06-09  7:05   ` Johannes Thumshirn
2021-06-10  6:02   ` Hannes Reinecke
2021-06-08 23:06 ` [PATCH 03/14] block/blk-rq-qos: Move a function from a header file into a C file Bart Van Assche
2021-06-09  4:22   ` Damien Le Moal
2021-06-10  6:03   ` Hannes Reinecke
2021-06-08 23:06 ` [PATCH 04/14] block: Introduce the ioprio rq-qos policy Bart Van Assche
2021-06-09  4:40   ` Damien Le Moal
2021-06-09 16:48     ` Bart Van Assche
2021-06-10  0:29       ` Damien Le Moal [this message]
2021-06-10  6:20   ` Hannes Reinecke
2021-06-10 17:14     ` Bart Van Assche
2021-06-08 23:06 ` [PATCH 05/14] block/mq-deadline: Add several comments Bart Van Assche
2021-06-10  6:21   ` Hannes Reinecke
2021-06-08 23:06 ` [PATCH 06/14] block/mq-deadline: Add two lockdep_assert_held() statements Bart Van Assche
2021-06-10  6:21   ` Hannes Reinecke
2021-06-08 23:06 ` [PATCH 07/14] block/mq-deadline: Remove two local variables Bart Van Assche
2021-06-10  6:22   ` Hannes Reinecke
2021-06-08 23:06 ` [PATCH 08/14] block/mq-deadline: Rename dd_init_queue() and dd_exit_queue() Bart Van Assche
2021-06-08 23:06 ` [PATCH 09/14] block/mq-deadline: Improve compile-time argument checking Bart Van Assche
2021-06-08 23:06 ` [PATCH 10/14] block/mq-deadline: Improve the sysfs show and store macros Bart Van Assche
2021-06-09  4:46   ` Damien Le Moal
2021-06-09 16:54     ` Bart Van Assche
2021-06-10  0:31       ` Damien Le Moal
2021-06-09  7:11   ` Johannes Thumshirn
2021-06-09 16:59     ` Bart Van Assche
2021-06-10  6:23   ` Hannes Reinecke
2021-06-08 23:07 ` [PATCH 11/14] block/mq-deadline: Reserve 25% of scheduler tags for synchronous requests Bart Van Assche
2021-06-10  6:26   ` Hannes Reinecke
2021-06-08 23:07 ` [PATCH 12/14] block/mq-deadline: Add I/O priority support Bart Van Assche
2021-06-09  5:03   ` Damien Le Moal
2021-06-09 17:25     ` Bart Van Assche
2021-06-10  0:39       ` Damien Le Moal
2021-06-10  6:35   ` Hannes Reinecke
2021-06-08 23:07 ` [PATCH 13/14] block/mq-deadline: Add cgroup support Bart Van Assche
2021-06-10  6:37   ` Hannes Reinecke
2021-06-08 23:07 ` [PATCH 14/14] block/mq-deadline: Prioritize high-priority requests Bart Van Assche
2021-06-09  5:10   ` Damien Le Moal
2021-06-10  6:43   ` Hannes Reinecke

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=DM6PR04MB7081960D31CBB02438C29D27E7359@DM6PR04MB7081.namprd04.prod.outlook.com \
    --to=damien.lemoal@wdc.com \
    --cc=Johannes.Thumshirn@wdc.com \
    --cc=axboe@kernel.dk \
    --cc=bvanassche@acm.org \
    --cc=hare@suse.de \
    --cc=hch@lst.de \
    --cc=himanshu.madhani@oracle.com \
    --cc=jaegeuk@kernel.org \
    --cc=linux-block@vger.kernel.org \
    --cc=ming.lei@redhat.com \
    --subject='Re: [PATCH 04/14] block: Introduce the ioprio rq-qos policy' \
    /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).