linux-kernel - Re: [RFC V2 PATCH 3/5] cfq-iosched: reimplement priorities using different service trees

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 21 Oct 2009 11:54:05 -0400
From:	Jeff Moyer <jmoyer@...hat.com>
To:	Corrado Zoccolo <czoccolo@...il.com>
Cc:	"Linux-Kernel" <linux-kernel@...r.kernel.org>,
	Jens Axboe <jens.axboe@...cle.com>
Subject: Re: [RFC V2 PATCH 3/5] cfq-iosched: reimplement priorities using different service trees

Corrado Zoccolo <czoccolo@...il.com> writes:

> We use different service trees for different priority classes.
> This allows a simplification in the service tree insertion code, that no
> longer has to consider priority while walking the tree.

This is kind of funny, considering things used to be divied up into
lists by class and priority.

> + * Index in the service_trees.
> + * IDLE is handled separately, so it has negative index
> + */
> +enum wl_prio_t {
> +	IDLE_WORKLOAD = -1,
> +	BE_WORKLOAD = 0,
> +	RT_WORKLOAD = 1
> +};

What's wrong with IOPRIO_CLASS_(RT|BE|IDLE)?  Why invent another enum?

> +
> +/*
>   * Per block device queue structure
>   */
>  struct cfq_data {
[...]
> +	struct cfq_rb_root service_trees[2];
> +	struct cfq_rb_root service_tree_idle;

Why separate out the idle service tree from the others?

> +static struct cfq_rb_root *service_tree_for(enum wl_prio_t prio,
> +					    struct cfq_data *cfqd)
> +{
> +	if (prio == IDLE_WORKLOAD)
> +		return &cfqd->service_tree_idle;
> +
> +	return &cfqd->service_trees[prio];
> +}

This should just turn into cfqd->service_trees[IOPRIO_CLASS_*] in the
callers.

[...]

>  /*
> @@ -1106,6 +1134,10 @@ static struct cfq_queue *cfq_close_cooperator(struct cfq_data *cfqd,
>  	if (cfq_cfqq_coop(cfqq))
>  		return NULL;
>  
> +	/* we don't want to mix processes with different characteristics */
> +	if (cfqq->service_tree != cur_cfqq->service_tree)
> +		return NULL;
> +

Hmm, that looks like a current bug in the close cooperator code.  It
shouldn't allow cooperation between differring scheduling classes.

Cheers,
Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/