diff mbox series

[7/8] ublk: rewrite ublk_ctrl_get_queue_affinity to not rely on hctx->cpumask

Message ID 20220721051632.1676890-8-hch@lst.de (mailing list archive)
State New, archived
Headers show
Series [1/8] ublk: add a MAINTAINERS entry | expand

Commit Message

Christoph Hellwig July 21, 2022, 5:16 a.m. UTC 
Looking at the hctxs and cpumap is not safe without at very last a RCU
reference.  It also requires the queue to be set up before starting the
device, which leads to rather awkware life time rules.

Instead rewrite ublk_ctrl_get_queue_affinity to call blk_mq_map_queues
on an on-stack blk_mq_queue_map and build the cpumask from that.

Note: given that ublk has not made it into a released kernel it might
make sense to change the ABI for this command to instead copy the
qmap.mq_map array (a nr_cpu_ids sized array of unsigned integer
values  where the CPU index directly points to the queue) to userspace.

That way all the information could be transported to userspace in a
single command.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/ublk_drv.c | 63 ++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 31 deletions(-)

Comments

Ming Lei July 21, 2022, 7:38 a.m. UTC | #1 
On Thu, Jul 21, 2022 at 07:16:31AM +0200, Christoph Hellwig wrote:
> Looking at the hctxs and cpumap is not safe without at very last a RCU
> reference.  It also requires the queue to be set up before starting the
> device, which leads to rather awkware life time rules.
> 
> Instead rewrite ublk_ctrl_get_queue_affinity to call blk_mq_map_queues
> on an on-stack blk_mq_queue_map and build the cpumask from that.
> 
> Note: given that ublk has not made it into a released kernel it might
> make sense to change the ABI for this command to instead copy the
> qmap.mq_map array (a nr_cpu_ids sized array of unsigned integer
> values  where the CPU index directly points to the queue) to userspace.

qmap.mq_map is too big.

tag_set is embedded into ublk_device, and can be allocated once during
adding device, then ublk_ctrl_get_queue_affinity() can retrieve the info from
tag_set's map directly, then it is simplified a lot.

Also all info for building tag_set is setup during adding device, and
these info won't be changed after adding device, so it is reasonable
to allocate tagset just once.


Thanks,
Ming
diff mbox series

Patch

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 7d57cbecfc8a0..67c6c46b8e07e 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1245,26 +1245,16 @@  static int ublk_ctrl_start_dev(struct io_uring_cmd *cmd)
 	return ret;
 }
 
-static struct blk_mq_hw_ctx *ublk_get_hw_queue(struct ublk_device *ub,
-		unsigned int index)
-{
-	struct blk_mq_hw_ctx *hctx;
-	unsigned long i;
-
-	queue_for_each_hw_ctx(ub->ub_queue, hctx, i)
-		if (hctx->queue_num == index)
-			return hctx;
-	return NULL;
-}
-
 static int ublk_ctrl_get_queue_affinity(struct io_uring_cmd *cmd)
 {
 	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
 	void __user *argp = (void __user *)(unsigned long)header->addr;
-	struct blk_mq_hw_ctx *hctx;
+	struct blk_mq_queue_map	qmap = {};
+	cpumask_var_t cpumask;
 	struct ublk_device *ub;
 	unsigned long queue;
 	unsigned int retlen;
+	unsigned int i;
 	int ret = -EINVAL;
 	
 	if (header->len * BITS_PER_BYTE < nr_cpu_ids)
@@ -1276,30 +1266,41 @@  static int ublk_ctrl_get_queue_affinity(struct io_uring_cmd *cmd)
 
 	ub = ublk_get_device_from_id(header->dev_id);
 	if (!ub)
-		goto out;
+		return -EINVAL;
 
 	queue = header->data[0];
 	if (queue >= ub->dev_info.nr_hw_queues)
-		goto out;
-	hctx = ublk_get_hw_queue(ub, queue);
-	if (!hctx)
-		goto out;
+		goto out_put_device;
+
+	qmap.mq_map = kcalloc(nr_cpu_ids, sizeof(qmap.mq_map), GFP_KERNEL);
+	if (!qmap.mq_map)
+		goto out_put_device;
+	qmap.nr_queues = ub->dev_info.nr_hw_queues;
+	blk_mq_map_queues(&qmap);
+
+	ret = -ENOMEM;
+	if (!zalloc_cpumask_var(&cpumask, GFP_KERNEL))
+		goto out_free_qmap;
+
+	for_each_possible_cpu(i)
+		if (qmap.mq_map[i] == queue)
+			cpumask_set_cpu(i, cpumask);
 
+	ret = -EFAULT;
 	retlen = min_t(unsigned short, header->len, cpumask_size());
-	if (copy_to_user(argp, hctx->cpumask, retlen)) {
-		ret = -EFAULT;
-		goto out;
-	}
-	if (retlen != header->len) {
-		if (clear_user(argp + retlen, header->len - retlen)) {
-			ret = -EFAULT;
-			goto out;
-		}
-	}
+	if (copy_to_user(argp, cpumask, retlen))
+		goto out_put_device;
+
+	if (retlen != header->len &&
+	    clear_user(argp + retlen, header->len - retlen))
+		goto out_put_device;
+
 	ret = 0;
- out:
-	if (ub)
-		ublk_put_device(ub);
+	free_cpumask_var(cpumask);
+out_free_qmap:
+	kfree(qmap.mq_map);
+out_put_device:
+	ublk_put_device(ub);
 	return ret;
 }