On 05.03.20 11:49, Roger Pau Monné wrote:
On Thu, Mar 05, 2020 at 11:03:31AM +0100, Juergen Gross wrote:
Commit 0265d6e8ddb890 ("xen/blkfront: limit allocated memory size to
actual use case") made struct blkfront_ring_info size dynamic. This is
fine when running with only one queue, but with multiple queues the
addressing of the single queues has to be adapted as the structs are
allocated in an array.
Thanks, and sorry for not catching this during review.
Fixes: 0265d6e8ddb890 ("xen/blkfront: limit allocated memory size to actual use case")
Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
---
drivers/block/xen-blkfront.c | 82 ++++++++++++++++++++++++--------------------
1 file changed, 45 insertions(+), 37 deletions(-)
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index e2ad6bba2281..a8d4a3838e5d 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -213,6 +213,7 @@ struct blkfront_info
struct blk_mq_tag_set tag_set;
struct blkfront_ring_info *rinfo;
unsigned int nr_rings;
+ unsigned int rinfo_size;
/* Save uncomplete reqs and bios for migration. */
struct list_head requests;
struct bio_list bio_list;
@@ -259,6 +260,21 @@ static int blkfront_setup_indirect(struct blkfront_ring_info *rinfo);
static void blkfront_gather_backend_features(struct blkfront_info *info);
static int negotiate_mq(struct blkfront_info *info);
+#define rinfo_ptr(rinfo, off) \
+ (struct blkfront_ring_info *)((unsigned long)(rinfo) + (off))
^ void * would seem more natural IMO.
Also if you use void * you don't need the extra (struct
blkfront_ring_info *) cast I think?
Yes, can change that.
I however think this macro is kind of weird, since it's just doing an
addition. I would rather have that calculation in get_rinfo and code
for_each_rinfo on top of that.
I wanted to avoid the multiplication in the rather common
for_each_rinfo() usage.
I agree this might be a question of taste, so I'm not going to insist
but that would reduce the number of helpers from 3 to 2.
+
+#define for_each_rinfo(info, rinfo, idx) \
+ for (rinfo = info->rinfo, idx = 0; \
+ idx < info->nr_rings; \
+ idx++, rinfo = rinfo_ptr(rinfo, info->rinfo_size))
I think the above is missing proper parentheses around macro
parameters.
rinfo and idx are simple variables, so I don't think they need
parentheses. info maybe. But just seeing it now: naming the
parameter "rinfo" and trying to access info->rinfo isn't a good
idea. It is working only as I always use "rinfo" as the pointer.
+
+static struct blkfront_ring_info *get_rinfo(struct blkfront_info *info,
+ unsigned int i)
inline attribute might be appropriate here.
See "the inline disease" in the kernel's coding style.
Juergen
