* [PATCH 1/1] Drivers: hv: vmbus: Calculate ring buffer size for more efficient use of memory @ 2024-02-13 6:19 mhkelley58 2024-02-20 6:30 ` Saurabh Singh Sengar ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: mhkelley58 @ 2024-02-13 6:19 UTC (permalink / raw) To: haiyangz, wei.liu, decui, boqun.feng, linux-kernel, linux-hyperv From: Michael Kelley <mhklinux@outlook.com> The VMBUS_RING_SIZE macro adds space for a ring buffer header to the requested ring buffer size. The header size is always 1 page, and so its size varies based on the PAGE_SIZE for which the kernel is built. If the requested ring buffer size is a large power-of-2 size and the header size is small, the resulting size is inefficient in its use of memory. For example, a 512 Kbyte ring buffer with a 4 Kbyte page size results in a 516 Kbyte allocation, which is rounded to up 1 Mbyte by the memory allocator, and wastes 508 Kbytes of memory. In such situations, the exact size of the ring buffer isn't that important, and it's OK to allocate the 4 Kbyte header at the beginning of the 512 Kbytes, leaving the ring buffer itself with just 508 Kbytes. The memory allocation can be 512 Kbytes instead of 1 Mbyte and nothing is wasted. Update VMBUS_RING_SIZE to implement this approach for "large" ring buffer sizes. "Large" is somewhat arbitrarily defined as 8 times the size of the ring buffer header (which is of size PAGE_SIZE). For example, for 4 Kbyte PAGE_SIZE, ring buffers of 32 Kbytes and larger use the first 4 Kbytes as the ring buffer header. For 64 Kbyte PAGE_SIZE, ring buffers of 512 Kbytes and larger use the first 64 Kbytes as the ring buffer header. In both cases, smaller sizes add space for the header so the ring size isn't reduced too much by using part of the space for the header. For example, with a 64 Kbyte page size, we don't want a 128 Kbyte ring buffer to be reduced to 64 Kbytes by allocating half of the space for the header. In such a case, the memory allocation is less efficient, but it's the best that can be done. Fixes: c1135c7fd0e9 ("Drivers: hv: vmbus: Introduce types of GPADL") Signed-off-by: Michael Kelley <mhklinux@outlook.com> --- include/linux/hyperv.h | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h index 2b00faf98017..6ef0557b4bff 100644 --- a/include/linux/hyperv.h +++ b/include/linux/hyperv.h @@ -164,8 +164,28 @@ struct hv_ring_buffer { u8 buffer[]; } __packed; + +/* + * If the requested ring buffer size is at least 8 times the size of the + * header, steal space from the ring buffer for the header. Otherwise, add + * space for the header so that is doesn't take too much of the ring buffer + * space. + * + * The factor of 8 is somewhat arbitrary. The goal is to prevent adding a + * relatively small header (4 Kbytes on x86) to a large-ish power-of-2 ring + * buffer size (such as 128 Kbytes) and so end up making a nearly twice as + * large allocation that will be almost half wasted. As a contrasting example, + * on ARM64 with 64 Kbyte page size, we don't want to take 64 Kbytes for the + * header from a 128 Kbyte allocation, leaving only 64 Kbytes for the ring. + * In this latter case, we must add 64 Kbytes for the header and not worry + * about what's wasted. + */ +#define VMBUS_HEADER_ADJ(payload_sz) \ + ((payload_sz) >= 8 * sizeof(struct hv_ring_buffer) ? \ + 0 : sizeof(struct hv_ring_buffer)) + /* Calculate the proper size of a ringbuffer, it must be page-aligned */ -#define VMBUS_RING_SIZE(payload_sz) PAGE_ALIGN(sizeof(struct hv_ring_buffer) + \ +#define VMBUS_RING_SIZE(payload_sz) PAGE_ALIGN(VMBUS_HEADER_ADJ(payload_sz) + \ (payload_sz)) struct hv_ring_buffer_info { -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] Drivers: hv: vmbus: Calculate ring buffer size for more efficient use of memory 2024-02-13 6:19 [PATCH 1/1] Drivers: hv: vmbus: Calculate ring buffer size for more efficient use of memory mhkelley58 @ 2024-02-20 6:30 ` Saurabh Singh Sengar 2024-02-20 17:29 ` Boqun Feng 2024-02-27 6:31 ` Dexuan Cui 2024-02-27 21:45 ` Souradeep Chakrabarti 2 siblings, 1 reply; 7+ messages in thread From: Saurabh Singh Sengar @ 2024-02-20 6:30 UTC (permalink / raw) To: mhklinux; +Cc: haiyangz, wei.liu, decui, boqun.feng, linux-kernel, linux-hyperv On Mon, Feb 12, 2024 at 10:19:59PM -0800, mhkelley58@gmail.com wrote: > From: Michael Kelley <mhklinux@outlook.com> > > The VMBUS_RING_SIZE macro adds space for a ring buffer header to the > requested ring buffer size. The header size is always 1 page, and so > its size varies based on the PAGE_SIZE for which the kernel is built. > If the requested ring buffer size is a large power-of-2 size and the header > size is small, the resulting size is inefficient in its use of memory. > For example, a 512 Kbyte ring buffer with a 4 Kbyte page size results in > a 516 Kbyte allocation, which is rounded to up 1 Mbyte by the memory > allocator, and wastes 508 Kbytes of memory. > > In such situations, the exact size of the ring buffer isn't that important, > and it's OK to allocate the 4 Kbyte header at the beginning of the 512 > Kbytes, leaving the ring buffer itself with just 508 Kbytes. The memory > allocation can be 512 Kbytes instead of 1 Mbyte and nothing is wasted. > > Update VMBUS_RING_SIZE to implement this approach for "large" ring buffer > sizes. "Large" is somewhat arbitrarily defined as 8 times the size of > the ring buffer header (which is of size PAGE_SIZE). For example, for > 4 Kbyte PAGE_SIZE, ring buffers of 32 Kbytes and larger use the first > 4 Kbytes as the ring buffer header. For 64 Kbyte PAGE_SIZE, ring buffers > of 512 Kbytes and larger use the first 64 Kbytes as the ring buffer > header. In both cases, smaller sizes add space for the header so > the ring size isn't reduced too much by using part of the space for > the header. For example, with a 64 Kbyte page size, we don't want > a 128 Kbyte ring buffer to be reduced to 64 Kbytes by allocating half > of the space for the header. In such a case, the memory allocation > is less efficient, but it's the best that can be done. > > Fixes: c1135c7fd0e9 ("Drivers: hv: vmbus: Introduce types of GPADL") > Signed-off-by: Michael Kelley <mhklinux@outlook.com> > --- > include/linux/hyperv.h | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h > index 2b00faf98017..6ef0557b4bff 100644 > --- a/include/linux/hyperv.h > +++ b/include/linux/hyperv.h > @@ -164,8 +164,28 @@ struct hv_ring_buffer { > u8 buffer[]; > } __packed; > > + > +/* > + * If the requested ring buffer size is at least 8 times the size of the > + * header, steal space from the ring buffer for the header. Otherwise, add > + * space for the header so that is doesn't take too much of the ring buffer > + * space. > + * > + * The factor of 8 is somewhat arbitrary. The goal is to prevent adding a > + * relatively small header (4 Kbytes on x86) to a large-ish power-of-2 ring > + * buffer size (such as 128 Kbytes) and so end up making a nearly twice as > + * large allocation that will be almost half wasted. As a contrasting example, > + * on ARM64 with 64 Kbyte page size, we don't want to take 64 Kbytes for the > + * header from a 128 Kbyte allocation, leaving only 64 Kbytes for the ring. > + * In this latter case, we must add 64 Kbytes for the header and not worry > + * about what's wasted. > + */ > +#define VMBUS_HEADER_ADJ(payload_sz) \ > + ((payload_sz) >= 8 * sizeof(struct hv_ring_buffer) ? \ > + 0 : sizeof(struct hv_ring_buffer)) > + > /* Calculate the proper size of a ringbuffer, it must be page-aligned */ > -#define VMBUS_RING_SIZE(payload_sz) PAGE_ALIGN(sizeof(struct hv_ring_buffer) + \ > +#define VMBUS_RING_SIZE(payload_sz) PAGE_ALIGN(VMBUS_HEADER_ADJ(payload_sz) + \ > (payload_sz)) > > struct hv_ring_buffer_info { Thanks for the patch. It's worth noting that this will affect the size of ringbuffer calculation for some of the drivers: netvsc, storvsc_drv, hid-hyperv, and hyperv-keyboard.c. It will be nice to have this comment added in commit for future reference. Looks a good improvement to me, Reviewed-by: Saurabh Sengar <ssengar@linux.microsoft.com> > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] Drivers: hv: vmbus: Calculate ring buffer size for more efficient use of memory 2024-02-20 6:30 ` Saurabh Singh Sengar @ 2024-02-20 17:29 ` Boqun Feng 2024-02-20 18:15 ` Michael Kelley 0 siblings, 1 reply; 7+ messages in thread From: Boqun Feng @ 2024-02-20 17:29 UTC (permalink / raw) To: Saurabh Singh Sengar Cc: mhklinux, haiyangz, wei.liu, decui, linux-kernel, linux-hyperv On Mon, Feb 19, 2024 at 10:30:07PM -0800, Saurabh Singh Sengar wrote: > On Mon, Feb 12, 2024 at 10:19:59PM -0800, mhkelley58@gmail.com wrote: > > From: Michael Kelley <mhklinux@outlook.com> > > > > The VMBUS_RING_SIZE macro adds space for a ring buffer header to the > > requested ring buffer size. The header size is always 1 page, and so > > its size varies based on the PAGE_SIZE for which the kernel is built. > > If the requested ring buffer size is a large power-of-2 size and the header > > size is small, the resulting size is inefficient in its use of memory. > > For example, a 512 Kbyte ring buffer with a 4 Kbyte page size results in > > a 516 Kbyte allocation, which is rounded to up 1 Mbyte by the memory > > allocator, and wastes 508 Kbytes of memory. > > > > In such situations, the exact size of the ring buffer isn't that important, > > and it's OK to allocate the 4 Kbyte header at the beginning of the 512 > > Kbytes, leaving the ring buffer itself with just 508 Kbytes. The memory > > allocation can be 512 Kbytes instead of 1 Mbyte and nothing is wasted. > > > > Update VMBUS_RING_SIZE to implement this approach for "large" ring buffer > > sizes. "Large" is somewhat arbitrarily defined as 8 times the size of > > the ring buffer header (which is of size PAGE_SIZE). For example, for > > 4 Kbyte PAGE_SIZE, ring buffers of 32 Kbytes and larger use the first > > 4 Kbytes as the ring buffer header. For 64 Kbyte PAGE_SIZE, ring buffers > > of 512 Kbytes and larger use the first 64 Kbytes as the ring buffer > > header. In both cases, smaller sizes add space for the header so > > the ring size isn't reduced too much by using part of the space for > > the header. For example, with a 64 Kbyte page size, we don't want > > a 128 Kbyte ring buffer to be reduced to 64 Kbytes by allocating half > > of the space for the header. In such a case, the memory allocation > > is less efficient, but it's the best that can be done. > > > > Fixes: c1135c7fd0e9 ("Drivers: hv: vmbus: Introduce types of GPADL") > > Signed-off-by: Michael Kelley <mhklinux@outlook.com> > > --- > > include/linux/hyperv.h | 22 +++++++++++++++++++++- > > 1 file changed, 21 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h > > index 2b00faf98017..6ef0557b4bff 100644 > > --- a/include/linux/hyperv.h > > +++ b/include/linux/hyperv.h > > @@ -164,8 +164,28 @@ struct hv_ring_buffer { > > u8 buffer[]; > > } __packed; > > > > + > > +/* > > + * If the requested ring buffer size is at least 8 times the size of the > > + * header, steal space from the ring buffer for the header. Otherwise, add > > + * space for the header so that is doesn't take too much of the ring buffer > > + * space. > > + * > > + * The factor of 8 is somewhat arbitrary. The goal is to prevent adding a > > + * relatively small header (4 Kbytes on x86) to a large-ish power-of-2 ring > > + * buffer size (such as 128 Kbytes) and so end up making a nearly twice as > > + * large allocation that will be almost half wasted. As a contrasting example, > > + * on ARM64 with 64 Kbyte page size, we don't want to take 64 Kbytes for the > > + * header from a 128 Kbyte allocation, leaving only 64 Kbytes for the ring. > > + * In this latter case, we must add 64 Kbytes for the header and not worry > > + * about what's wasted. > > + */ > > +#define VMBUS_HEADER_ADJ(payload_sz) \ > > + ((payload_sz) >= 8 * sizeof(struct hv_ring_buffer) ? \ > > + 0 : sizeof(struct hv_ring_buffer)) > > + > > /* Calculate the proper size of a ringbuffer, it must be page-aligned */ > > -#define VMBUS_RING_SIZE(payload_sz) PAGE_ALIGN(sizeof(struct hv_ring_buffer) + \ > > +#define VMBUS_RING_SIZE(payload_sz) PAGE_ALIGN(VMBUS_HEADER_ADJ(payload_sz) + \ > > (payload_sz)) I generally see the point of this patch, however, it changes the semantics of VMBUS_RING_SIZE() (similiar as Saurabh mentioned below), before VMBUS_RING_SIZE() will give you a ring buffer size which has at least "payload_sz" bytes, but after the change, you may not get "enough" bytes for the vmbus ring buffer. One cause of the waste memory is using alloc_pages() to get physical continuous, however, after a quick look into GPADL, looks like it also supports uncontinuous pages. Maybe that's the longer-term solution? Regards, Boqun > > > > struct hv_ring_buffer_info { > > Thanks for the patch. > It's worth noting that this will affect the size of ringbuffer calculation for > some of the drivers: netvsc, storvsc_drv, hid-hyperv, and hyperv-keyboard.c. > It will be nice to have this comment added in commit for future reference. > > Looks a good improvement to me, > Reviewed-by: Saurabh Sengar <ssengar@linux.microsoft.com> > > > -- > > 2.25.1 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH 1/1] Drivers: hv: vmbus: Calculate ring buffer size for more efficient use of memory 2024-02-20 17:29 ` Boqun Feng @ 2024-02-20 18:15 ` Michael Kelley 2024-02-26 4:25 ` Boqun Feng 0 siblings, 1 reply; 7+ messages in thread From: Michael Kelley @ 2024-02-20 18:15 UTC (permalink / raw) To: Boqun Feng, Saurabh Singh Sengar Cc: haiyangz, wei.liu, decui, linux-kernel, linux-hyperv From: Boqun Feng <boqun.feng@gmail.com> Sent: Tuesday, February 20, 2024 9:29 AM > > On Mon, Feb 19, 2024 at 10:30:07PM -0800, Saurabh Singh Sengar wrote: > > On Mon, Feb 12, 2024 at 10:19:59PM -0800, mhkelley58@gmail.com wrote: > > > From: Michael Kelley <mhklinux@outlook.com> > > > > > > The VMBUS_RING_SIZE macro adds space for a ring buffer header to the > > > requested ring buffer size. The header size is always 1 page, and so > > > its size varies based on the PAGE_SIZE for which the kernel is built. > > > If the requested ring buffer size is a large power-of-2 size and the header > > > size is small, the resulting size is inefficient in its use of memory. > > > For example, a 512 Kbyte ring buffer with a 4 Kbyte page size results in > > > a 516 Kbyte allocation, which is rounded to up 1 Mbyte by the memory > > > allocator, and wastes 508 Kbytes of memory. > > > > > > In such situations, the exact size of the ring buffer isn't that important, > > > and it's OK to allocate the 4 Kbyte header at the beginning of the 512 > > > Kbytes, leaving the ring buffer itself with just 508 Kbytes. The memory > > > allocation can be 512 Kbytes instead of 1 Mbyte and nothing is wasted. > > > > > > Update VMBUS_RING_SIZE to implement this approach for "large" ring buffer > > > sizes. "Large" is somewhat arbitrarily defined as 8 times the size of > > > the ring buffer header (which is of size PAGE_SIZE). For example, for > > > 4 Kbyte PAGE_SIZE, ring buffers of 32 Kbytes and larger use the first > > > 4 Kbytes as the ring buffer header. For 64 Kbyte PAGE_SIZE, ring buffers > > > of 512 Kbytes and larger use the first 64 Kbytes as the ring buffer > > > header. In both cases, smaller sizes add space for the header so > > > the ring size isn't reduced too much by using part of the space for > > > the header. For example, with a 64 Kbyte page size, we don't want > > > a 128 Kbyte ring buffer to be reduced to 64 Kbytes by allocating half > > > of the space for the header. In such a case, the memory allocation > > > is less efficient, but it's the best that can be done. > > > > > > Fixes: c1135c7fd0e9 ("Drivers: hv: vmbus: Introduce types of GPADL") > > > Signed-off-by: Michael Kelley <mhklinux@outlook.com> > > > --- > > > include/linux/hyperv.h | 22 +++++++++++++++++++++- > > > 1 file changed, 21 insertions(+), 1 deletion(-) > > > > > > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h > > > index 2b00faf98017..6ef0557b4bff 100644 > > > --- a/include/linux/hyperv.h > > > +++ b/include/linux/hyperv.h > > > @@ -164,8 +164,28 @@ struct hv_ring_buffer { > > > u8 buffer[]; > > > } __packed; > > > > > > + > > > +/* > > > + * If the requested ring buffer size is at least 8 times the size of the > > > + * header, steal space from the ring buffer for the header. Otherwise, add > > > + * space for the header so that is doesn't take too much of the ring buffer > > > + * space. > > > + * > > > + * The factor of 8 is somewhat arbitrary. The goal is to prevent adding a > > > + * relatively small header (4 Kbytes on x86) to a large-ish power-of-2 ring > > > + * buffer size (such as 128 Kbytes) and so end up making a nearly twice as > > > + * large allocation that will be almost half wasted. As a contrasting example, > > > + * on ARM64 with 64 Kbyte page size, we don't want to take 64 Kbytes for the > > > + * header from a 128 Kbyte allocation, leaving only 64 Kbytes for the ring. > > > + * In this latter case, we must add 64 Kbytes for the header and not worry > > > + * about what's wasted. > > > + */ > > > +#define VMBUS_HEADER_ADJ(payload_sz) \ > > > + ((payload_sz) >= 8 * sizeof(struct hv_ring_buffer) ? \ > > > + 0 : sizeof(struct hv_ring_buffer)) > > > + > > > /* Calculate the proper size of a ringbuffer, it must be page-aligned */ > > > -#define VMBUS_RING_SIZE(payload_sz) PAGE_ALIGN(sizeof(struct hv_ring_buffer) + \ > > > +#define VMBUS_RING_SIZE(payload_sz) PAGE_ALIGN(VMBUS_HEADER_ADJ(payload_sz) + \ > > > (payload_sz)) > > I generally see the point of this patch, however, it changes the > semantics of VMBUS_RING_SIZE() (similiar as Saurabh mentioned below), > before VMBUS_RING_SIZE() will give you a ring buffer size which has at > least "payload_sz" bytes, but after the change, you may not get "enough" > bytes for the vmbus ring buffer. Storvsc and netvsc were previously not using VMBUS_RING_SIZE(), so space for the ring buffer header was already being "stolen" from the specified ring size. But if this new version of VMBUS_RING_SIZE() still allows the ring buffer header space to be "stolen" from the large-ish ring buffers, I don't see that as a problem. The ring buffer sizes have always been a swag value for the low-speed devices with small ring buffers. For the high-speed devices (netvsc and storvsc) the ring buffer sizes have been somewhat determined by perf measurements, but the ring buffers are much bigger, so stealing a little bit of space doesn't have any noticeable effect. Even with the perf measurements, the sizes aren't very exact. Performance just isn't sensitive to the size of the ring buffer except at a gross level. > > One cause of the waste memory is using alloc_pages() to get physical > continuous, however, after a quick look into GPADL, looks like it also > supports uncontinuous pages. Maybe that's the longer-term solution? Yes, that seems like a desirable long-term solution. While the GPADL code handles vmalloc memory correctly (because the netvsc send and receive buffers are vmalloc memory), hv_ringbuffer_init() assumes physically contiguous memory. It would need to use vmalloc_to_page() in building the pages_wraparound array instead of just indexing into the struct page array. But that's an easy fix. Another problem is the uio_hv_generic.c driver, where hv_uio_ring_mmap() assumes a physically contiguous ring. Maybe there's a straightforward way to fix it as well, but it isn't immediately obvious to me. Using vmalloc memory for ring buffers has another benefit as well. Today, adding a new NIC to a VM that has been running for a while could fail because of not being able to allocate 1 Mbyte contiguous memory for the ring buffers used by each VMBus channel. There could be plenty of memory available, but fragmentation could prevent getting 1 Mbyte contiguous. Using vmalloc'ed ring buffers would solve this problem. Michael > > Regards, > Boqun > > > > > > > struct hv_ring_buffer_info { > > > > Thanks for the patch. > > It's worth noting that this will affect the size of ringbuffer calculation for > > some of the drivers: netvsc, storvsc_drv, hid-hyperv, and hyperv-keyboard.c. > > It will be nice to have this comment added in commit for future reference. > > > > Looks a good improvement to me, > > Reviewed-by: Saurabh Sengar <ssengar@linux.microsoft.com> > > > > > -- > > > 2.25.1 > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] Drivers: hv: vmbus: Calculate ring buffer size for more efficient use of memory 2024-02-20 18:15 ` Michael Kelley @ 2024-02-26 4:25 ` Boqun Feng 0 siblings, 0 replies; 7+ messages in thread From: Boqun Feng @ 2024-02-26 4:25 UTC (permalink / raw) To: Michael Kelley Cc: Saurabh Singh Sengar, haiyangz, wei.liu, decui, linux-kernel, linux-hyperv On Tue, Feb 20, 2024 at 06:15:52PM +0000, Michael Kelley wrote: > From: Boqun Feng <boqun.feng@gmail.com> Sent: Tuesday, February 20, 2024 9:29 AM > > > > On Mon, Feb 19, 2024 at 10:30:07PM -0800, Saurabh Singh Sengar wrote: > > > On Mon, Feb 12, 2024 at 10:19:59PM -0800, mhkelley58@gmail.com wrote: > > > > From: Michael Kelley <mhklinux@outlook.com> > > > > > > > > The VMBUS_RING_SIZE macro adds space for a ring buffer header to the > > > > requested ring buffer size. The header size is always 1 page, and so > > > > its size varies based on the PAGE_SIZE for which the kernel is built. > > > > If the requested ring buffer size is a large power-of-2 size and the header > > > > size is small, the resulting size is inefficient in its use of memory. > > > > For example, a 512 Kbyte ring buffer with a 4 Kbyte page size results in > > > > a 516 Kbyte allocation, which is rounded to up 1 Mbyte by the memory > > > > allocator, and wastes 508 Kbytes of memory. > > > > > > > > In such situations, the exact size of the ring buffer isn't that important, > > > > and it's OK to allocate the 4 Kbyte header at the beginning of the 512 > > > > Kbytes, leaving the ring buffer itself with just 508 Kbytes. The memory > > > > allocation can be 512 Kbytes instead of 1 Mbyte and nothing is wasted. > > > > > > > > Update VMBUS_RING_SIZE to implement this approach for "large" ring buffer > > > > sizes. "Large" is somewhat arbitrarily defined as 8 times the size of > > > > the ring buffer header (which is of size PAGE_SIZE). For example, for > > > > 4 Kbyte PAGE_SIZE, ring buffers of 32 Kbytes and larger use the first > > > > 4 Kbytes as the ring buffer header. For 64 Kbyte PAGE_SIZE, ring buffers > > > > of 512 Kbytes and larger use the first 64 Kbytes as the ring buffer > > > > header. In both cases, smaller sizes add space for the header so > > > > the ring size isn't reduced too much by using part of the space for > > > > the header. For example, with a 64 Kbyte page size, we don't want > > > > a 128 Kbyte ring buffer to be reduced to 64 Kbytes by allocating half > > > > of the space for the header. In such a case, the memory allocation > > > > is less efficient, but it's the best that can be done. > > > > > > > > Fixes: c1135c7fd0e9 ("Drivers: hv: vmbus: Introduce types of GPADL") > > > > Signed-off-by: Michael Kelley <mhklinux@outlook.com> > > > > --- > > > > include/linux/hyperv.h | 22 +++++++++++++++++++++- > > > > 1 file changed, 21 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h > > > > index 2b00faf98017..6ef0557b4bff 100644 > > > > --- a/include/linux/hyperv.h > > > > +++ b/include/linux/hyperv.h > > > > @@ -164,8 +164,28 @@ struct hv_ring_buffer { > > > > u8 buffer[]; > > > > } __packed; > > > > > > > > + > > > > +/* > > > > + * If the requested ring buffer size is at least 8 times the size of the > > > > + * header, steal space from the ring buffer for the header. Otherwise, add > > > > + * space for the header so that is doesn't take too much of the ring buffer > > > > + * space. > > > > + * > > > > + * The factor of 8 is somewhat arbitrary. The goal is to prevent adding a > > > > + * relatively small header (4 Kbytes on x86) to a large-ish power-of-2 ring > > > > + * buffer size (such as 128 Kbytes) and so end up making a nearly twice as > > > > + * large allocation that will be almost half wasted. As a contrasting example, > > > > + * on ARM64 with 64 Kbyte page size, we don't want to take 64 Kbytes for the > > > > + * header from a 128 Kbyte allocation, leaving only 64 Kbytes for the ring. > > > > + * In this latter case, we must add 64 Kbytes for the header and not worry > > > > + * about what's wasted. > > > > + */ > > > > +#define VMBUS_HEADER_ADJ(payload_sz) \ > > > > + ((payload_sz) >= 8 * sizeof(struct hv_ring_buffer) ? \ > > > > + 0 : sizeof(struct hv_ring_buffer)) > > > > + > > > > /* Calculate the proper size of a ringbuffer, it must be page-aligned */ > > > > -#define VMBUS_RING_SIZE(payload_sz) PAGE_ALIGN(sizeof(struct hv_ring_buffer) + \ > > > > +#define VMBUS_RING_SIZE(payload_sz) PAGE_ALIGN(VMBUS_HEADER_ADJ(payload_sz) + \ > > > > (payload_sz)) > > > > I generally see the point of this patch, however, it changes the > > semantics of VMBUS_RING_SIZE() (similiar as Saurabh mentioned below), > > before VMBUS_RING_SIZE() will give you a ring buffer size which has at > > least "payload_sz" bytes, but after the change, you may not get "enough" > > bytes for the vmbus ring buffer. > > Storvsc and netvsc were previously not using VMBUS_RING_SIZE(), > so space for the ring buffer header was already being "stolen" from > the specified ring size. But if this new version of VMBUS_RING_SIZE() > still allows the ring buffer header space to be "stolen" from the > large-ish ring buffers, I don't see that as a problem. The ring buffer > sizes have always been a swag value for the low-speed devices with > small ring buffers. For the high-speed devices (netvsc and storvsc) > the ring buffer sizes have been somewhat determined by perf > measurements, but the ring buffers are much bigger, so stealing > a little bit of space doesn't have any noticeable effect. Even with > the perf measurements, the sizes aren't very exact. Performance > just isn't sensitive to the size of the ring buffer except at a gross level. > Fair enough. > > > > One cause of the waste memory is using alloc_pages() to get physical > > continuous, however, after a quick look into GPADL, looks like it also > > supports uncontinuous pages. Maybe that's the longer-term solution? > > Yes, that seems like a desirable long-term solution. While the GPADL > code handles vmalloc memory correctly (because the netvsc send and > receive buffers are vmalloc memory), hv_ringbuffer_init() assumes > physically contiguous memory. It would need to use vmalloc_to_page() > in building the pages_wraparound array instead of just indexing into > the struct page array. But that's an easy fix. Another problem is the > uio_hv_generic.c driver, where hv_uio_ring_mmap() assumes a physically > contiguous ring. Maybe there's a straightforward way to fix it as well, > but it isn't immediately obvious to me. > > Using vmalloc memory for ring buffers has another benefit as well. > Today, adding a new NIC to a VM that has been running for a while > could fail because of not being able to allocate 1 Mbyte contiguous > memory for the ring buffers used by each VMBus channel. There > could be plenty of memory available, but fragmentation could prevent > getting 1 Mbyte contiguous. Using vmalloc'ed ring buffers would > solve this problem. > Yep, that will be ideal. Regards, Boqun > Michael > > > > > Regards, > > Boqun > > > > > > > > > > struct hv_ring_buffer_info { > > > > > > Thanks for the patch. > > > It's worth noting that this will affect the size of ringbuffer calculation for > > > some of the drivers: netvsc, storvsc_drv, hid-hyperv, and hyperv-keyboard.c. > > > It will be nice to have this comment added in commit for future reference. > > > > > > Looks a good improvement to me, > > > Reviewed-by: Saurabh Sengar <ssengar@linux.microsoft.com> > > > > > > > -- > > > > 2.25.1 > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH 1/1] Drivers: hv: vmbus: Calculate ring buffer size for more efficient use of memory 2024-02-13 6:19 [PATCH 1/1] Drivers: hv: vmbus: Calculate ring buffer size for more efficient use of memory mhkelley58 2024-02-20 6:30 ` Saurabh Singh Sengar @ 2024-02-27 6:31 ` Dexuan Cui 2024-02-27 21:45 ` Souradeep Chakrabarti 2 siblings, 0 replies; 7+ messages in thread From: Dexuan Cui @ 2024-02-27 6:31 UTC (permalink / raw) To: mhklinux, Haiyang Zhang, wei.liu, boqun.feng, linux-kernel, linux-hyperv Cc: Souradeep Chakrabarti > From: mhkelley58@gmail.com <mhkelley58@gmail.com> > Sent: Monday, February 12, 2024 10:20 PM > [...] > Fixes: c1135c7fd0e9 ("Drivers: hv: vmbus: Introduce types of GPADL") > Signed-off-by: Michael Kelley <mhklinux@outlook.com> > --- Reviewed-by: Dexuan Cui <decui@microsoft.com> The info below would need to be added: Fixes: 6941f67ad37d ("hv_netvsc: Calculate correct ring size when PAGE_SIZE is not 4 Kbytes") Link: https://bugzilla.kernel.org/show_bug.cgi?id=218502 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/1] Drivers: hv: vmbus: Calculate ring buffer size for more efficient use of memory 2024-02-13 6:19 [PATCH 1/1] Drivers: hv: vmbus: Calculate ring buffer size for more efficient use of memory mhkelley58 2024-02-20 6:30 ` Saurabh Singh Sengar 2024-02-27 6:31 ` Dexuan Cui @ 2024-02-27 21:45 ` Souradeep Chakrabarti 2 siblings, 0 replies; 7+ messages in thread From: Souradeep Chakrabarti @ 2024-02-27 21:45 UTC (permalink / raw) To: mhkelley58 Cc: boqun.feng, decui, haiyangz, linux-hyperv, linux-kernel, mhklinux, wei.liu Tested-by: Souradeep Chakrabarti <schakrabarti@linux.microsoft.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-02-27 21:45 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-02-13 6:19 [PATCH 1/1] Drivers: hv: vmbus: Calculate ring buffer size for more efficient use of memory mhkelley58 2024-02-20 6:30 ` Saurabh Singh Sengar 2024-02-20 17:29 ` Boqun Feng 2024-02-20 18:15 ` Michael Kelley 2024-02-26 4:25 ` Boqun Feng 2024-02-27 6:31 ` Dexuan Cui 2024-02-27 21:45 ` Souradeep Chakrabarti
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).