[PATCH 1/2] Drivers: hv: vmbus: Remove duplication and cleanup code in create_gpadl_header()

[PATCH 1/2] Drivers: hv: vmbus: Remove duplication and cleanup code in create_gpadl_header()

[mhkelley58]

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

[PATCH 2/2] Drivers: hv: vmbus: Update indentation in create_gpadl_header()

[mhkelley58]

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

Re: [PATCH 1/2] Drivers: hv: vmbus: Remove duplication and cleanup code in create_gpadl_header()

[Markus Elfring]

…
> 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

RE: [PATCH 1/2] Drivers: hv: vmbus: Remove duplication and cleanup code in create_gpadl_header()

[Michael Kelley]

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

RE: [PATCH 1/2] Drivers: hv: vmbus: Remove duplication and cleanup code in create_gpadl_header()

[Michael Kelley]

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

Re: [PATCH 1/2] Drivers: hv: vmbus: Remove duplication and cleanup code in create_gpadl_header()

[Wei Liu]

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.