> Subject: Re: [PATCH v4] tools/hv: fcopy: Fix irregularities with size of ring > buffer > > > > On 7/11/2025 11:38 AM, Naman Jain wrote: > > 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: [email protected] > > Fixes: 0315fef2aff9 ("uio_hv_generic: Align ring size to system page") > > Signed-off-by: Naman Jain <[email protected]> > > Reviewed-by: Saurabh Sengar <[email protected]>
Reviewed-by: Long Li <[email protected]> > > --- > > Noticed that I missed adding change logs (again). Adding them now. > > Changes since v3: > https://lore.kern/ > el.org%2Fall%2F20250708080319.3904-1- > namjain%40linux.microsoft.com%2F&data=05%7C02%7Clongli%40microsoft.com > %7C5061a1ac57814cbe542b08ddc042614e%7C72f988bf86f141af91ab2d7cd011 > db47%7C1%7C0%7C638878113544016320%7CUnknown%7CTWFpbGZsb3d8eyJ > FbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWF > pbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=loiQdbSCzLluXmFECiIrrHbbK9 > fqxJBbLTSAB26%2Bd0k%3D&reserved=0 > * Added a goto label for freeing desc memory. (Saurabh) > * Avoided declaring device path twice by using FCOPY_DEVICE_PATH(subdir) > (Saurabh) > * Removed extra len variable assignment in main() (Saurabh) > * added Reviewed-by tag from Saurabh > > Changes since v2: > https://lore.kern/ > el.org%2Fall%2F20250701104837.3006-1- > namjain%40linux.microsoft.com%2F&data=05%7C02%7Clongli%40microsoft.com > %7C5061a1ac57814cbe542b08ddc042614e%7C72f988bf86f141af91ab2d7cd011 > db47%7C1%7C0%7C638878113544051861%7CUnknown%7CTWFpbGZsb3d8eyJ > FbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWF > pbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=PZ9GdNL7%2FD6NfxZPhCOX > 7zQ2DbspDgQs%2BS75PFGAFG4%3D&reserved=0 > * Removed fallback mechanism to default size, to keep fcopy behavior > consistent > (Long's suggestion). If ring sysfs file is not present for some reason, > things are > already bad and its the right thing for fcopy to abort. > > Changes since v1: > https://lore.kern/ > el.org%2Fall%2F20250620070618.3097-1- > namjain%40linux.microsoft.com%2F&data=05%7C02%7Clongli%40microsoft.com > %7C5061a1ac57814cbe542b08ddc042614e%7C72f988bf86f141af91ab2d7cd011 > db47%7C1%7C0%7C638878113544063634%7CUnknown%7CTWFpbGZsb3d8eyJ > FbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWF > pbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=LQsSaTjbam4DPAzMQmy1% > 2BMB0%2BKb6L7d3xKkM954mpls%3D&reserved=0 > > * 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) > > > > > tools/hv/hv_fcopy_uio_daemon.c | 91 ++++++++++++++++++++++++++++++-- > -- > > 1 file changed, 81 insertions(+), 10 deletions(-) > > > > diff --git a/tools/hv/hv_fcopy_uio_daemon.c > > b/tools/hv/hv_fcopy_uio_daemon.c index 0198321d14a2..7d9bcb066d3f > > 100644 > > --- a/tools/hv/hv_fcopy_uio_daemon.c > > +++ b/tools/hv/hv_fcopy_uio_daemon.c > > @@ -35,7 +35,10 @@ > > #define WIN8_SRV_MINOR 1 > > #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_DEVICE_PATH(subdir) \ > > + "/sys/bus/vmbus/devices/eb765408-105f-49b6-b4aa-c123b64d17d4/" > #subdir > > +#define FCOPY_UIO_PATH FCOPY_DEVICE_PATH(uio) > > +#define FCOPY_CHANNELS_PATH FCOPY_DEVICE_PATH(channels) > > > > #define FCOPY_VER_COUNT 1 > > static const int fcopy_versions[] = { @@ -47,9 +50,62 @@ static > > const int fw_versions[] = { > > UTIL_FW_VERSION > > }; > > > > -#define HV_RING_SIZE 0x4000 /* 16KB ring buffer 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 0; > > + } > > + } > > + > > +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; > > + } > > + } > > + } > > > > -static unsigned char desc[HV_RING_SIZE]; > > + 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) > > + syslog(LOG_ERR, "Could not determine ring size"); > > + > > + return ring_size; > > +} > > + > > +static unsigned char *desc; > > > > static int target_fd; > > static char target_fname[PATH_MAX]; > > @@ -406,7 +462,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,7 +493,20 @@ int main(int argc, char *argv[]) > > openlog("HV_UIO_FCOPY", 0, LOG_USER); > > syslog(LOG_INFO, "starting; pid is:%d", getpid()); > > > > - fcopy_get_first_folder(FCOPY_UIO, uio_name); > > + ring_size = get_ring_buffer_size(); > > + if (!ring_size) { > > + ret = -ENODEV; > > + goto exit; > > + } > > + > > + 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_PATH, uio_name); > > snprintf(uio_dev_path, sizeof(uio_dev_path), "/dev/%s", uio_name); > > fcopy_fd = open(uio_dev_path, O_RDWR); > > > > @@ -445,17 +514,17 @@ int main(int argc, char *argv[]) > > syslog(LOG_ERR, "open %s failed; error: %d %s", > > uio_dev_path, errno, strerror(errno)); > > ret = fcopy_fd; > > - goto exit; > > + goto free_desc; > > } > > > > - 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 +541,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) */ > @@ > > -492,6 +561,8 @@ int main(int argc, char *argv[]) > > } > > close: > > close(fcopy_fd); > > +free_desc: > > + free(desc); > > exit: > > return ret; > > } > > > > base-commit: b551c4e2a98a177a06148cf16505643cd2108386
