linux-kernel - Re: [PATCH 06/12] blkcg: use q and plid instead of opaque void * for blkio

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]

Message-ID: <20120119140442.GA9582@redhat.com>
Date:	Thu, 19 Jan 2012 09:04:42 -0500
From:	Vivek Goyal <vgoyal@...hat.com>
To:	Tejun Heo <tj@...nel.org>
Cc:	axboe@...nel.dk, ctalbott@...gle.com, rni@...gle.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 06/12] blkcg: use q and plid instead of opaque void * for
 blkio_group association

On Wed, Jan 18, 2012 at 05:11:24PM -0800, Tejun Heo wrote:
> blkgio_group is association between a block cgroup and a queue for a
> given policy.  Using opaque void * for association makes things
> confusing and hinders factoring of common code.  Use request_queue *
> and, if necessary, policy id instead.
> 
> This will help block cgroup API cleanup.

Using void* allowed one to pass any type of data pointer as key by the
client.

I think passing cfq_data or throtl_data as key is better than passing
request queue as key.

- During elevator exit, it looks like there will be a small window where
  groups from both old elevator and new elevator will be present in blkcg
  list. Given the fact that there can be only one active elevator at a
  time, during cgroup removal call there is no way to reach a group's
  cfqd. One can only retrieve request queue reliably and can't rely
  on q->elevator->elevator_data.

  So passing cfq_data as key provides more flexibility and allows
  co-existence of two elevators more naturally without information loss.

[..]
> -static void cfq_unlink_blkio_group(void *key, struct blkio_group *blkg)
> +static void cfq_unlink_blkio_group(struct request_queue *q,
> +				   struct blkio_group *blkg)
>  {
> -	unsigned long  flags;
> -	struct cfq_data *cfqd = key;
> +	struct cfq_data *cfqd = q->elevator->elevator_data;
> +	unsigned long flags;
>  
> -	spin_lock_irqsave(cfqd->queue->queue_lock, flags);
> +	spin_lock_irqsave(q->queue_lock, flags);
>  	cfq_destroy_cfqg(cfqd, cfqg_of_blkg(blkg));
> -	spin_unlock_irqrestore(cfqd->queue->queue_lock, flags);
> +	spin_unlock_irqrestore(q->queue_lock, flags);

I think this code will create problem where both old elevator group and
new elevator group is on blkcg list and upon cgroup removal one can not
rely that q->elevator->elevator_data will give us old elevator's cfqd.

Having said that, in practice we might never hit it as elevator init time
we only initialize and connect root group in blkcg list and one can not
delete root cgroup so above function is never called for root group.

But I think it is confusing so it is probably better to register cfq_data
or throtl_data as key instead of request queue.

Thanks
Vivek
--
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/