* [PATCH 1/2] Drivers: hv: vmbus: Remove duplication and cleanup code in create_gpadl_header()
@ 2024-01-11 16:54 mhkelley58
2024-01-11 16:54 ` [PATCH 2/2] Drivers: hv: vmbus: Update indentation " mhkelley58
2024-01-12 8:06 ` [PATCH 1/2] Drivers: hv: vmbus: Remove duplication and cleanup code " Markus Elfring
0 siblings, 2 replies; 6+ messages in thread
From: mhkelley58 @ 2024-01-11 16:54 UTC (permalink / raw)
To: haiyangz, wei.liu, decui, dan.carpenter, Markus.Elfring,
linux-kernel, linux-hyperv
From: Michael Kelley <mhklinux@outlook.com>
create_gpadl_header() creates a message header, and one or more message
bodies if the number of GPADL entries exceeds what fits in the
header. Currently the code for creating the message header is
duplicated in the two halves of the main "if" statement governing
whether message bodies are created.
Eliminate the duplication by making minor tweaks to the logic and
associated comments. While here, simplify the handling of memory
allocation errors, and use umin() instead of open coding it.
For ease of review, the indentation of sizable chunks of code is
*not* changed. A follow-on patch updates only the indentation.
No functional change.
Suggested-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
---
Because the GPADL messages interact with Hyper-V and use
PFNs, I tested on x86, ARM64 with 4K page size, and ARM64
with 64K page size. All look good.
drivers/hv/channel.c | 54 ++++++++------------------------------------
1 file changed, 10 insertions(+), 44 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 56f7e06c673e..604f5aff8502 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -322,21 +322,17 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
pagecount = hv_gpadl_size(type, size) >> HV_HYP_PAGE_SHIFT;
- /* do we need a gpadl body msg */
pfnsize = MAX_SIZE_CHANNEL_MESSAGE -
sizeof(struct vmbus_channel_gpadl_header) -
sizeof(struct gpa_range);
- pfncount = pfnsize / sizeof(u64);
+ pfncount = umin(pagecount, pfnsize / sizeof(u64));
- if (pagecount > pfncount) {
- /* we need a gpadl body */
- /* fill in the header */
msgsize = sizeof(struct vmbus_channel_msginfo) +
sizeof(struct vmbus_channel_gpadl_header) +
sizeof(struct gpa_range) + pfncount * sizeof(u64);
msgheader = kzalloc(msgsize, GFP_KERNEL);
if (!msgheader)
- goto nomem;
+ return -ENOMEM;
INIT_LIST_HEAD(&msgheader->submsglist);
msgheader->msgsize = msgsize;
@@ -356,18 +352,17 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
pfnsum = pfncount;
pfnleft = pagecount - pfncount;
- /* how many pfns can we fit */
+ /* how many pfns can we fit in a body message */
pfnsize = MAX_SIZE_CHANNEL_MESSAGE -
sizeof(struct vmbus_channel_gpadl_body);
pfncount = pfnsize / sizeof(u64);
- /* fill in the body */
+ /*
+ * If pfnleft is zero, everything fits in the header and no body
+ * messages are needed
+ */
while (pfnleft) {
- if (pfnleft > pfncount)
- pfncurr = pfncount;
- else
- pfncurr = pfnleft;
-
+ pfncurr = umin(pfncount, pfnleft);
msgsize = sizeof(struct vmbus_channel_msginfo) +
sizeof(struct vmbus_channel_gpadl_body) +
pfncurr * sizeof(u64);
@@ -386,8 +381,8 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
list_del(&pos->msglistentry);
kfree(pos);
}
-
- goto nomem;
+ kfree(msgheader);
+ return -ENOMEM;
}
msgbody->msgsize = msgsize;
@@ -410,37 +405,8 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
pfnsum += pfncurr;
pfnleft -= pfncurr;
}
- } else {
- /* everything fits in a header */
- msgsize = sizeof(struct vmbus_channel_msginfo) +
- sizeof(struct vmbus_channel_gpadl_header) +
- sizeof(struct gpa_range) + pagecount * sizeof(u64);
- msgheader = kzalloc(msgsize, GFP_KERNEL);
- if (msgheader == NULL)
- goto nomem;
-
- INIT_LIST_HEAD(&msgheader->submsglist);
- msgheader->msgsize = msgsize;
-
- gpadl_header = (struct vmbus_channel_gpadl_header *)
- msgheader->msg;
- gpadl_header->rangecount = 1;
- gpadl_header->range_buflen = sizeof(struct gpa_range) +
- pagecount * sizeof(u64);
- gpadl_header->range[0].byte_offset = 0;
- gpadl_header->range[0].byte_count = hv_gpadl_size(type, size);
- for (i = 0; i < pagecount; i++)
- gpadl_header->range[0].pfn_array[i] = hv_gpadl_hvpfn(
- type, kbuffer, size, send_offset, i);
-
- *msginfo = msgheader;
- }
return 0;
-nomem:
- kfree(msgheader);
- kfree(msgbody);
- return -ENOMEM;
}
/*
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] Drivers: hv: vmbus: Update indentation in create_gpadl_header()
2024-01-11 16:54 [PATCH 1/2] Drivers: hv: vmbus: Remove duplication and cleanup code in create_gpadl_header() mhkelley58
@ 2024-01-11 16:54 ` mhkelley58
2024-01-12 8:06 ` [PATCH 1/2] Drivers: hv: vmbus: Remove duplication and cleanup code " Markus Elfring
1 sibling, 0 replies; 6+ messages in thread
From: mhkelley58 @ 2024-01-11 16:54 UTC (permalink / raw)
To: haiyangz, wei.liu, decui, dan.carpenter, Markus.Elfring,
linux-kernel, linux-hyperv
From: Michael Kelley <mhklinux@outlook.com>
A previous commit left the indentation in create_gpadl_header()
unchanged for ease of review. Update the indentation and remove
line wrap in two places where it is no longer necessary.
No functional change.
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
---
drivers/hv/channel.c | 142 +++++++++++++++++++++----------------------
1 file changed, 70 insertions(+), 72 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 604f5aff8502..adbf674355b2 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -327,85 +327,83 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
sizeof(struct gpa_range);
pfncount = umin(pagecount, pfnsize / sizeof(u64));
- msgsize = sizeof(struct vmbus_channel_msginfo) +
- sizeof(struct vmbus_channel_gpadl_header) +
- sizeof(struct gpa_range) + pfncount * sizeof(u64);
- msgheader = kzalloc(msgsize, GFP_KERNEL);
- if (!msgheader)
- return -ENOMEM;
-
- INIT_LIST_HEAD(&msgheader->submsglist);
- msgheader->msgsize = msgsize;
-
- gpadl_header = (struct vmbus_channel_gpadl_header *)
- msgheader->msg;
- gpadl_header->rangecount = 1;
- gpadl_header->range_buflen = sizeof(struct gpa_range) +
- pagecount * sizeof(u64);
- gpadl_header->range[0].byte_offset = 0;
- gpadl_header->range[0].byte_count = hv_gpadl_size(type, size);
- for (i = 0; i < pfncount; i++)
- gpadl_header->range[0].pfn_array[i] = hv_gpadl_hvpfn(
- type, kbuffer, size, send_offset, i);
- *msginfo = msgheader;
-
- pfnsum = pfncount;
- pfnleft = pagecount - pfncount;
-
- /* how many pfns can we fit in a body message */
- pfnsize = MAX_SIZE_CHANNEL_MESSAGE -
- sizeof(struct vmbus_channel_gpadl_body);
- pfncount = pfnsize / sizeof(u64);
+ msgsize = sizeof(struct vmbus_channel_msginfo) +
+ sizeof(struct vmbus_channel_gpadl_header) +
+ sizeof(struct gpa_range) + pfncount * sizeof(u64);
+ msgheader = kzalloc(msgsize, GFP_KERNEL);
+ if (!msgheader)
+ return -ENOMEM;
- /*
- * If pfnleft is zero, everything fits in the header and no body
- * messages are needed
- */
- while (pfnleft) {
- pfncurr = umin(pfncount, pfnleft);
- msgsize = sizeof(struct vmbus_channel_msginfo) +
- sizeof(struct vmbus_channel_gpadl_body) +
- pfncurr * sizeof(u64);
- msgbody = kzalloc(msgsize, GFP_KERNEL);
-
- if (!msgbody) {
- struct vmbus_channel_msginfo *pos = NULL;
- struct vmbus_channel_msginfo *tmp = NULL;
- /*
- * Free up all the allocated messages.
- */
- list_for_each_entry_safe(pos, tmp,
- &msgheader->submsglist,
- msglistentry) {
-
- list_del(&pos->msglistentry);
- kfree(pos);
- }
- kfree(msgheader);
- return -ENOMEM;
- }
+ INIT_LIST_HEAD(&msgheader->submsglist);
+ msgheader->msgsize = msgsize;
+
+ gpadl_header = (struct vmbus_channel_gpadl_header *)
+ msgheader->msg;
+ gpadl_header->rangecount = 1;
+ gpadl_header->range_buflen = sizeof(struct gpa_range) +
+ pagecount * sizeof(u64);
+ gpadl_header->range[0].byte_offset = 0;
+ gpadl_header->range[0].byte_count = hv_gpadl_size(type, size);
+ for (i = 0; i < pfncount; i++)
+ gpadl_header->range[0].pfn_array[i] = hv_gpadl_hvpfn(
+ type, kbuffer, size, send_offset, i);
+ *msginfo = msgheader;
+
+ pfnsum = pfncount;
+ pfnleft = pagecount - pfncount;
+
+ /* how many pfns can we fit in a body message */
+ pfnsize = MAX_SIZE_CHANNEL_MESSAGE -
+ sizeof(struct vmbus_channel_gpadl_body);
+ pfncount = pfnsize / sizeof(u64);
- msgbody->msgsize = msgsize;
- gpadl_body =
- (struct vmbus_channel_gpadl_body *)msgbody->msg;
+ /*
+ * If pfnleft is zero, everything fits in the header and no body
+ * messages are needed
+ */
+ while (pfnleft) {
+ pfncurr = umin(pfncount, pfnleft);
+ msgsize = sizeof(struct vmbus_channel_msginfo) +
+ sizeof(struct vmbus_channel_gpadl_body) +
+ pfncurr * sizeof(u64);
+ msgbody = kzalloc(msgsize, GFP_KERNEL);
+ if (!msgbody) {
+ struct vmbus_channel_msginfo *pos = NULL;
+ struct vmbus_channel_msginfo *tmp = NULL;
/*
- * Gpadl is u32 and we are using a pointer which could
- * be 64-bit
- * This is governed by the guest/host protocol and
- * so the hypervisor guarantees that this is ok.
+ * Free up all the allocated messages.
*/
- for (i = 0; i < pfncurr; i++)
- gpadl_body->pfn[i] = hv_gpadl_hvpfn(type,
- kbuffer, size, send_offset, pfnsum + i);
-
- /* add to msg header */
- list_add_tail(&msgbody->msglistentry,
- &msgheader->submsglist);
- pfnsum += pfncurr;
- pfnleft -= pfncurr;
+ list_for_each_entry_safe(pos, tmp,
+ &msgheader->submsglist,
+ msglistentry) {
+
+ list_del(&pos->msglistentry);
+ kfree(pos);
+ }
+ kfree(msgheader);
+ return -ENOMEM;
}
+ msgbody->msgsize = msgsize;
+ gpadl_body = (struct vmbus_channel_gpadl_body *)msgbody->msg;
+
+ /*
+ * Gpadl is u32 and we are using a pointer which could
+ * be 64-bit
+ * This is governed by the guest/host protocol and
+ * so the hypervisor guarantees that this is ok.
+ */
+ for (i = 0; i < pfncurr; i++)
+ gpadl_body->pfn[i] = hv_gpadl_hvpfn(type,
+ kbuffer, size, send_offset, pfnsum + i);
+
+ /* add to msg header */
+ list_add_tail(&msgbody->msglistentry, &msgheader->submsglist);
+ pfnsum += pfncurr;
+ pfnleft -= pfncurr;
+ }
+
return 0;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] Drivers: hv: vmbus: Remove duplication and cleanup code in create_gpadl_header()
2024-01-11 16:54 [PATCH 1/2] Drivers: hv: vmbus: Remove duplication and cleanup code in create_gpadl_header() mhkelley58
2024-01-11 16:54 ` [PATCH 2/2] Drivers: hv: vmbus: Update indentation " mhkelley58
@ 2024-01-12 8:06 ` Markus Elfring
2024-01-12 16:19 ` Michael Kelley
1 sibling, 1 reply; 6+ messages in thread
From: Markus Elfring @ 2024-01-12 8:06 UTC (permalink / raw)
To: linux-hyperv, Michael Kelley, Dexuan Cui, Haiyang Zhang, Wei Liu,
Dan Carpenter, kernel-janitors
Cc: LKML
…
> Eliminate the duplication by making minor tweaks to the logic and
> associated comments. While here, simplify the handling of memory
> allocation errors, and use umin() instead of open coding it.
…
I got the impression that the adjustment for the mentioned macro
should be performed in a separate update step of the presented patch series.
https://elixir.bootlin.com/linux/v6.7/source/include/linux/minmax.h#L95
See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.7#n81
Regards,
Markus
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 1/2] Drivers: hv: vmbus: Remove duplication and cleanup code in create_gpadl_header()
2024-01-12 8:06 ` [PATCH 1/2] Drivers: hv: vmbus: Remove duplication and cleanup code " Markus Elfring
@ 2024-01-12 16:19 ` Michael Kelley
2024-02-09 15:33 ` Michael Kelley
0 siblings, 1 reply; 6+ messages in thread
From: Michael Kelley @ 2024-01-12 16:19 UTC (permalink / raw)
To: Markus Elfring, linux-hyperv@vger.kernel.org, Dexuan Cui,
Haiyang Zhang, Wei Liu, Dan Carpenter,
kernel-janitors@vger.kernel.org
Cc: LKML
From: Markus Elfring <Markus.Elfring@web.de> Sent: Friday, January 12, 2024 12:06 AM
>
> …
> > Eliminate the duplication by making minor tweaks to the logic and
> > associated comments. While here, simplify the handling of memory
> > allocation errors, and use umin() instead of open coding it.
> …
>
> I got the impression that the adjustment for the mentioned macro
> should be performed in a separate update step of the presented patch series.
> https://elixir.bootlin.com/linux/v6.7/source/include/linux/minmax.h#L95
>
> See also:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu
> mentation/process/submitting-patches.rst?h=v6.7#n81
>
To me, this is a judgment call. Breaking out the umin() change into
a separate patch is OK, but for consistency then I should probably
break out the change to memory allocation errors in the same
way. Then we would have three patches, plus the patch to
separately handle the indentation so the changes are reviewable.
To me, that's overkill for updates to a single function that have
no functionality change. The intent of the patch is to cleanup
and simplify a single 13-year old function, and it's OK to do
that in a single patch (plus the indentation patch).
Wei Liu is the maintainer for the Hyper-V code. Wei -- any
objections to keeping a single patch (plus the indentation patch)?
But I'll break it out if that's your preference.
Michael
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 1/2] Drivers: hv: vmbus: Remove duplication and cleanup code in create_gpadl_header()
2024-01-12 16:19 ` Michael Kelley
@ 2024-02-09 15:33 ` Michael Kelley
2024-03-01 9:25 ` Wei Liu
0 siblings, 1 reply; 6+ messages in thread
From: Michael Kelley @ 2024-02-09 15:33 UTC (permalink / raw)
To: Wei Liu
Cc: LKML, Michael Kelley, Markus Elfring,
linux-hyperv@vger.kernel.org, Dexuan Cui, Haiyang Zhang,
Dan Carpenter, kernel-janitors@vger.kernel.org
From: Michael Kelley <mhklinux@outlook.com> Sent: Friday, January 12, 2024 8:19 AM
>
> From: Markus Elfring <Markus.Elfring@web.de> Sent: Friday, January 12,
> 2024 12:06 AM
> >
> > …
> > > Eliminate the duplication by making minor tweaks to the logic and
> > > associated comments. While here, simplify the handling of memory
> > > allocation errors, and use umin() instead of open coding it.
> > …
> >
> > I got the impression that the adjustment for the mentioned macro
> > should be performed in a separate update step of the presented patch series.
> > https://elixir.bootlin.com/linux/v6.7/source/include/linux/minmax.h#L95
> >
> > See also:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu
> > mentation/process/submitting-patches.rst?h=v6.7#n81
> >
>
> To me, this is a judgment call. Breaking out the umin() change into
> a separate patch is OK, but for consistency then I should probably
> break out the change to memory allocation errors in the same
> way. Then we would have three patches, plus the patch to
> separately handle the indentation so the changes are reviewable.
> To me, that's overkill for updates to a single function that have
> no functionality change. The intent of the patch is to cleanup
> and simplify a single 13-year old function, and it's OK to do
> that in a single patch (plus the indentation patch).
>
> Wei Liu is the maintainer for the Hyper-V code. Wei -- any
> objections to keeping a single patch (plus the indentation patch)?
> But I'll break it out if that's your preference.
>
Wei Liu -- any input on this? This is just a cleanup/simplification
patch, so it's not urgent.
Michael
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] Drivers: hv: vmbus: Remove duplication and cleanup code in create_gpadl_header()
2024-02-09 15:33 ` Michael Kelley
@ 2024-03-01 9:25 ` Wei Liu
0 siblings, 0 replies; 6+ messages in thread
From: Wei Liu @ 2024-03-01 9:25 UTC (permalink / raw)
To: Michael Kelley
Cc: Wei Liu, LKML, Markus Elfring, linux-hyperv@vger.kernel.org,
Dexuan Cui, Haiyang Zhang, Dan Carpenter,
kernel-janitors@vger.kernel.org
On Fri, Feb 09, 2024 at 03:33:24PM +0000, Michael Kelley wrote:
> From: Michael Kelley <mhklinux@outlook.com> Sent: Friday, January 12, 2024 8:19 AM
> >
> > From: Markus Elfring <Markus.Elfring@web.de> Sent: Friday, January 12,
> > 2024 12:06 AM
> > >
> > > …
> > > > Eliminate the duplication by making minor tweaks to the logic and
> > > > associated comments. While here, simplify the handling of memory
> > > > allocation errors, and use umin() instead of open coding it.
> > > …
> > >
> > > I got the impression that the adjustment for the mentioned macro
> > > should be performed in a separate update step of the presented patch series.
> > > https://elixir.bootlin.com/linux/v6.7/source/include/linux/minmax.h#L95
> > >
> > > See also:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu
> > > mentation/process/submitting-patches.rst?h=v6.7#n81
> > >
> >
> > To me, this is a judgment call. Breaking out the umin() change into
> > a separate patch is OK, but for consistency then I should probably
> > break out the change to memory allocation errors in the same
> > way. Then we would have three patches, plus the patch to
> > separately handle the indentation so the changes are reviewable.
> > To me, that's overkill for updates to a single function that have
> > no functionality change. The intent of the patch is to cleanup
> > and simplify a single 13-year old function, and it's OK to do
> > that in a single patch (plus the indentation patch).
> >
> > Wei Liu is the maintainer for the Hyper-V code. Wei -- any
> > objections to keeping a single patch (plus the indentation patch)?
> > But I'll break it out if that's your preference.
> >
>
> Wei Liu -- any input on this? This is just a cleanup/simplification
> patch, so it's not urgent.
These patches are fine. I'll take them via the hyperv-fixes tree.
Thanks,
Wei.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-03-01 9:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-11 16:54 [PATCH 1/2] Drivers: hv: vmbus: Remove duplication and cleanup code in create_gpadl_header() mhkelley58
2024-01-11 16:54 ` [PATCH 2/2] Drivers: hv: vmbus: Update indentation " mhkelley58
2024-01-12 8:06 ` [PATCH 1/2] Drivers: hv: vmbus: Remove duplication and cleanup code " Markus Elfring
2024-01-12 16:19 ` Michael Kelley
2024-02-09 15:33 ` Michael Kelley
2024-03-01 9:25 ` Wei Liu
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).