Re: [PATCH 1/6] Drivers: hv: vmbus: Add utility function for querying ring size

Saurabh Singh Sengar
2024-02-18T08:03:06Z

On Sun, Feb 18, 2024 at 08:11:58AM +0100, Greg KH wrote:
> On Sat, Feb 17, 2024 at 10:03:35AM -0800, Saurabh Sengar wrote:
> > Add a function to query for the preferred ring buffer size of VMBus
> > device.
>
> That says what you did, but not why you did it.

I thought subsequent patch will make it clear, but I can add more
info in cover letter. I will enhance this commit as well.

>
> >
> > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> > ---
> > drivers/hv/channel_mgmt.c | 7 +++++--
> > drivers/hv/hyperv_vmbus.h | 5 +++++
> > include/linux/hyperv.h | 1 +
> > 3 files changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> > index 2f4d09ce027a..7ea444d72f9f 100644
> > --- a/drivers/hv/channel_mgmt.c
> > +++ b/drivers/hv/channel_mgmt.c
> > @@ -120,7 +120,8 @@ const struct vmbus_device vmbus_devs[] = {
> > },
> >
> > /* File copy */
> > - { .dev_type = HV_FCOPY,
> > + { .pref_ring_size = 0x4000,
> > + .dev_type = HV_FCOPY,
> > HV_FCOPY_GUID,
> > .perf_device = false,
> > .allowed_in_isolated = false,
> > @@ -141,11 +142,13 @@ const struct vmbus_device vmbus_devs[] = {
> > },
> >
> > /* Unknown GUID */
> > - { .dev_type = HV_UNKNOWN,
> > + { .pref_ring_size = 0x11000,
> > + .dev_type = HV_UNKNOWN,
>
> Where do these magic numbers for the size come from?

This value is (16 + 1) page_size, which is found sufficient for most
of the slow devices. 16 page_size is for the ringbuffer and 1 page_size
for the headre. This is the approximation for default case, to avoid
using fall back case of 512 page_size as used by uio_hv_generic.

>
> > .perf_device = false,
> > .allowed_in_isolated = false,> > },
> > };
> > +EXPORT_SYMBOL_GPL(vmbus_devs);
> >
> > static const struct {
> > guid_t guid;
> > diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> > index f6b1e710f805..76ac5185a01a 100644
> > --- a/drivers/hv/hyperv_vmbus.h
> > +++ b/drivers/hv/hyperv_vmbus.h
> > @@ -417,6 +417,11 @@ static inline bool hv_is_perf_channel(struct vmbus_channel *channel)
> > return vmbus_devs[channel->device_id].perf_device;
> > }
> >
> > +static inline size_t hv_dev_ring_size(struct vmbus_channel *channel)
> > +{
> > + return vmbus_devs[channel->device_id].pref_ring_size;
> > +}
> > +
> > static inline bool hv_is_allocated_cpu(unsigned int cpu)
> > {
> > struct vmbus_channel *channel, *sc;
> > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> > index 2b00faf98017..5951c7bb5712 100644
> > --- a/include/linux/hyperv.h
> > +++ b/include/linux/hyperv.h
> > @@ -800,6 +800,7 @@ struct vmbus_requestor {
> > #define VMBUS_RQST_RESET (U64_MAX - 3)
> >
> > struct vmbus_device {
> > + size_t pref_ring_size;
>
> No documentation for this? What is the size in units of?

I can add a comment here like below:

/*
* Total memory in bytes allocated for the one complete ring buffer,
* which includes the ring buffer header, of size PAGE_SIZE. This value
* should be aligned to page_size.
*/

- Saurabh

>
> thanks,
>
> greg k-h