diff mbox series

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

Message ID 1708193020-14740-2-git-send-email-ssengar@linux.microsoft.com
State New
Headers
Series Low speed Hyper-V devices support |

Commit Message

Saurabh Singh Sengar Feb. 17, 2024, 6:03 p.m. UTC 
  Add a function to query for the preferred ring buffer size of VMBus
device.

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(-)

Comments

Greg KH Feb. 18, 2024, 7:11 a.m. UTC | #1 
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.

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

>  	  .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?

thanks,

greg k-h
Saurabh Singh Sengar Feb. 18, 2024, 8:03 a.m. UTC | #2 
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
Greg KH Feb. 18, 2024, 9:11 a.m. UTC | #3 
On Sun, Feb 18, 2024 at 12:03:06AM -0800, Saurabh Singh Sengar wrote:
> 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.

Each patch should stand on its own, as it will be on its own when
committed, right?

I don't know anything is happening "next", nor what any of this means
here, which is required.

Again, what would _you_ want to see if you had to review this?

Along those lines, why not get some internal review, and signed-off-by
first, before asking us to review this for you?  You all know this area
the best, and have lots of experience with reviews, right?

thanks,

greg k-h
diff mbox series

Patch

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,
 	  .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;
 	u16  dev_type;
 	guid_t guid;
 	bool perf_device;