[PATCH v2] tools/hv: fcopy: Fix irregularities with size of ring buffer

[PATCH v2] tools/hv: fcopy: Fix irregularities with size of ring buffer

[Naman Jain]

Size of ring buffer, as defined in uio_hv_generic driver, is no longer
fixed to 16 KB. This creates a problem in fcopy, since this size was
hardcoded. With the change in place to make ring sysfs node actually
reflect the size of underlying ring buffer, it is safe to get the size
of ring sysfs file and use it for ring buffer size in fcopy daemon.
Fix the issue of disparity in ring buffer size, by making it dynamic
in fcopy uio daemon.

Cc: stable@vger.kernel.org
Fixes: 0315fef2aff9 ("uio_hv_generic: Align ring size to system page")
Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
---
Changes since v1:
https://lore.kernel.org/all/20250620070618.3097-1-namjain@linux.microsoft.com/

* Removed unnecessary type casting in malloc for desc variable (Olaf)
* Added retry mechanisms to avoid potential race conditions (Michael)
* Moved the logic to fetch ring size to a later part in main (Michael)

So Michael, you suggested me to move the above logic after the
/dev/uio<N> entry is successfully opened. But there is some issue
with uio_hv_generic changing interrupt mask, resulting in fcopy daemon
to get stuck in pread forever. Since fcopy is broken as of now, I
thought it is better to take it separately, and let this change go.
Please let me know if you are fine with that.
---
 tools/hv/hv_fcopy_uio_daemon.c | 82 +++++++++++++++++++++++++++++++---
 1 file changed, 75 insertions(+), 7 deletions(-)

diff --git a/tools/hv/hv_fcopy_uio_daemon.c b/tools/hv/hv_fcopy_uio_daemon.c
index 0198321d14a2..f2e4976ebf28 100644
--- a/tools/hv/hv_fcopy_uio_daemon.c
+++ b/tools/hv/hv_fcopy_uio_daemon.c
@@ -36,6 +36,7 @@
 #define WIN8_SRV_VERSION	(WIN8_SRV_MAJOR << 16 | WIN8_SRV_MINOR)
 
 #define FCOPY_UIO		"/sys/bus/vmbus/devices/eb765408-105f-49b6-b4aa-c123b64d17d4/uio"
+#define FCOPY_CHANNELS_PATH	"/sys/bus/vmbus/devices/eb765408-105f-49b6-b4aa-c123b64d17d4/channels"
 
 #define FCOPY_VER_COUNT		1
 static const int fcopy_versions[] = {
@@ -47,9 +48,67 @@ static const int fw_versions[] = {
 	UTIL_FW_VERSION
 };
 
-#define HV_RING_SIZE		0x4000 /* 16KB ring buffer size */
+#define HV_RING_SIZE_DEFAULT	0x4000 /* 16KB ring buffer size default */
 
-static unsigned char desc[HV_RING_SIZE];
+static uint32_t get_ring_buffer_size(void)
+{
+	char ring_path[PATH_MAX];
+	DIR *dir;
+	struct dirent *entry;
+	struct stat st;
+	uint32_t ring_size = 0;
+	int retry_count = 0;
+
+	/* Find the channel directory */
+	dir = opendir(FCOPY_CHANNELS_PATH);
+	if (!dir) {
+		usleep(100 * 1000); /* Avoid race with kernel, wait 100ms and retry once */
+		dir = opendir(FCOPY_CHANNELS_PATH);
+		if (!dir) {
+			syslog(LOG_ERR, "Failed to open channels directory: %s", strerror(errno));
+			return HV_RING_SIZE_DEFAULT;
+		}
+	}
+
+retry_once:
+	while ((entry = readdir(dir)) != NULL) {
+		if (entry->d_type == DT_DIR && strcmp(entry->d_name, ".") != 0 &&
+		    strcmp(entry->d_name, "..") != 0) {
+			snprintf(ring_path, sizeof(ring_path), "%s/%s/ring",
+				 FCOPY_CHANNELS_PATH, entry->d_name);
+
+			if (stat(ring_path, &st) == 0) {
+				/*
+				 * stat returns size of Tx, Rx rings combined,
+				 * so take half of it for individual ring size.
+				 */
+				ring_size = (uint32_t)st.st_size / 2;
+				syslog(LOG_INFO, "Ring buffer size from %s: %u bytes",
+				       ring_path, ring_size);
+				break;
+			}
+		}
+	}
+
+	if (!ring_size && retry_count == 0) {
+		retry_count = 1;
+		rewinddir(dir);
+		usleep(100 * 1000); /* Wait 100ms and retry once */
+		goto retry_once;
+	}
+
+	closedir(dir);
+
+	if (!ring_size) {
+		ring_size = HV_RING_SIZE_DEFAULT;
+		syslog(LOG_ERR, "Could not determine ring size, using default: %u bytes",
+		       HV_RING_SIZE_DEFAULT);
+	}
+
+	return ring_size;
+}
+
+static unsigned char *desc;
 
 static int target_fd;
 static char target_fname[PATH_MAX];
@@ -406,7 +465,7 @@ int main(int argc, char *argv[])
 	int daemonize = 1, long_index = 0, opt, ret = -EINVAL;
 	struct vmbus_br txbr, rxbr;
 	void *ring;
-	uint32_t len = HV_RING_SIZE;
+	uint32_t ring_size, len;
 	char uio_name[NAME_MAX] = {0};
 	char uio_dev_path[PATH_MAX] = {0};
 
@@ -437,6 +496,15 @@ int main(int argc, char *argv[])
 	openlog("HV_UIO_FCOPY", 0, LOG_USER);
 	syslog(LOG_INFO, "starting; pid is:%d", getpid());
 
+	ring_size = get_ring_buffer_size();
+	len = ring_size;
+	desc = malloc(ring_size * sizeof(unsigned char));
+	if (!desc) {
+		syslog(LOG_ERR, "malloc failed for desc buffer");
+		ret = -ENOMEM;
+		goto exit;
+	}
+
 	fcopy_get_first_folder(FCOPY_UIO, uio_name);
 	snprintf(uio_dev_path, sizeof(uio_dev_path), "/dev/%s", uio_name);
 	fcopy_fd = open(uio_dev_path, O_RDWR);
@@ -448,14 +516,14 @@ int main(int argc, char *argv[])
 		goto exit;
 	}
 
-	ring = vmbus_uio_map(&fcopy_fd, HV_RING_SIZE);
+	ring = vmbus_uio_map(&fcopy_fd, ring_size);
 	if (!ring) {
 		ret = errno;
 		syslog(LOG_ERR, "mmap ringbuffer failed; error: %d %s", ret, strerror(ret));
 		goto close;
 	}
-	vmbus_br_setup(&txbr, ring, HV_RING_SIZE);
-	vmbus_br_setup(&rxbr, (char *)ring + HV_RING_SIZE, HV_RING_SIZE);
+	vmbus_br_setup(&txbr, ring, ring_size);
+	vmbus_br_setup(&rxbr, (char *)ring + ring_size, ring_size);
 
 	rxbr.vbr->imask = 0;
 
@@ -472,7 +540,7 @@ int main(int argc, char *argv[])
 			goto close;
 		}
 
-		len = HV_RING_SIZE;
+		len = ring_size;
 		ret = rte_vmbus_chan_recv_raw(&rxbr, desc, &len);
 		if (unlikely(ret <= 0)) {
 			/* This indicates a failure to communicate (or worse) */

base-commit: 1343433ed38923a21425c602e92120a1f1db5f7a
-- 
2.34.1

Re: [PATCH v2] tools/hv: fcopy: Fix irregularities with size of ring buffer

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 285 bytes --]

Tue,  1 Jul 2025 16:18:37 +0530 Naman Jain <namjain@linux.microsoft.com>:

> +		syslog(LOG_ERR, "Could not determine ring size, using default: %u bytes",
> +		       HV_RING_SIZE_DEFAULT);

I think this is not an actionable error.
Maybe use the default just silently?


Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] tools/hv: fcopy: Fix irregularities with size of ring buffer

[Naman Jain]

On 7/1/2025 4:45 PM, Olaf Hering wrote:
> Tue,  1 Jul 2025 16:18:37 +0530 Naman Jain <namjain@linux.microsoft.com>:
> 
>> +		syslog(LOG_ERR, "Could not determine ring size, using default: %u bytes",
>> +		       HV_RING_SIZE_DEFAULT);
> 
> I think this is not an actionable error.
> Maybe use the default just silently?
> 
> 
> Olaf

So let's suppose a case, where the actual ring buffer size was different
than the default value, and for some reason, we were not able to
determine the ring size from the sysfs entry. This results in wrong size
configured in FCopy daemon. FCopy does not work in that scenario and
silently fails. We would definitely like to inform that to the user.
Default size case is just to provide a best effort way of making it
work.
I am fine to change it to LOG_INFO or keep it the same. Please let me
know your thoughts.

Regards,
Naman

RE: [EXTERNAL] Re: [PATCH v2] tools/hv: fcopy: Fix irregularities with size of ring buffer

[Long Li]

> Subject: [EXTERNAL] Re: [PATCH v2] tools/hv: fcopy: Fix irregularities with size of
> ring buffer
> 
> Tue,  1 Jul 2025 16:18:37 +0530 Naman Jain <namjain@linux.microsoft.com>:
> 
> > +		syslog(LOG_ERR, "Could not determine ring size, using
> default: %u bytes",
> > +		       HV_RING_SIZE_DEFAULT);
> 
> I think this is not an actionable error.
> Maybe use the default just silently?
> 

How about just fail fcopy?

This will have a consistent behavior.

Long

Re: [EXTERNAL] Re: [PATCH v2] tools/hv: fcopy: Fix irregularities with size of ring buffer

[Naman Jain]

On 7/2/2025 12:02 PM, Long Li wrote:
>> Subject: [EXTERNAL] Re: [PATCH v2] tools/hv: fcopy: Fix irregularities with size of
>> ring buffer
>>
>> Tue,  1 Jul 2025 16:18:37 +0530 Naman Jain <namjain@linux.microsoft.com>:
>>
>>> +		syslog(LOG_ERR, "Could not determine ring size, using
>> default: %u bytes",
>>> +		       HV_RING_SIZE_DEFAULT);
>>
>> I think this is not an actionable error.
>> Maybe use the default just silently?
>>
> 
> How about just fail fcopy?
> 
> This will have a consistent behavior.
> 
> Long

I am OK with that as well. I provided an explanation regarding best
effort fallback mechanism in my other reply in this thread.

 From silently ignoring it (1) to failing fcopy (2), or simply logging 
as info (3), I would personally prefer either (2) or (3) or keep it in
current form. I'll wait for this discussion to conclude though since we
have varied opinions on this.

Regards,
Naman