vhost: Fix typos in comments and clarity on alignof usage

[PATCH] vhost: Fix typos in comments and clarity on alignof usage

Posted by Alok Tiwari 3 months, 3 weeks ago

This patch fixes multiple typos and improves comment clarity across
vhost.c.
- Correct spelling errors: "thead" -> "thread", "RUNNUNG" -> "RUNNING"
  and "available".
- Improve comment by replacing informal comment ("Supersize me!")
  with a clear description.
- Use __alignof__ correctly on dereferenced pointer types for better
  readability and alignment with kernel documentation.

These changes enhance code readability and maintainability.

Signed-off-by: Alok Tiwari <alok.a.tiwari@oracle.com>
---
 drivers/vhost/vhost.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 3a5ebb973dba..0227c123c0e0 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -594,10 +594,10 @@ static void vhost_attach_mm(struct vhost_dev *dev)
 	if (dev->use_worker) {
 		dev->mm = get_task_mm(current);
 	} else {
-		/* vDPA device does not use worker thead, so there's
-		 * no need to hold the address space for mm. This help
+		/* vDPA device does not use worker thread, so there's
+		 * no need to hold the address space for mm. This helps
 		 * to avoid deadlock in the case of mmap() which may
-		 * held the refcnt of the file and depends on release
+		 * hold the refcnt of the file and depends on release
 		 * method to remove vma.
 		 */
 		dev->mm = current->mm;
@@ -731,7 +731,7 @@ static void __vhost_vq_attach_worker(struct vhost_virtqueue *vq,
 	 * We don't want to call synchronize_rcu for every vq during setup
 	 * because it will slow down VM startup. If we haven't done
 	 * VHOST_SET_VRING_KICK and not done the driver specific
-	 * SET_ENDPOINT/RUNNUNG then we can skip the sync since there will
+	 * SET_ENDPOINT/RUNNING then we can skip the sync since there will
 	 * not be any works queued for scsi and net.
 	 */
 	mutex_lock(&vq->mutex);
@@ -1898,8 +1898,8 @@ static long vhost_vring_set_addr(struct vhost_dev *d,
 		return -EFAULT;
 
 	/* Make sure it's safe to cast pointers to vring types. */
-	BUILD_BUG_ON(__alignof__ *vq->avail > VRING_AVAIL_ALIGN_SIZE);
-	BUILD_BUG_ON(__alignof__ *vq->used > VRING_USED_ALIGN_SIZE);
+	BUILD_BUG_ON(__alignof__(*vq->avail) > VRING_AVAIL_ALIGN_SIZE);
+	BUILD_BUG_ON(__alignof__(*vq->used) > VRING_USED_ALIGN_SIZE);
 	if ((a.avail_user_addr & (VRING_AVAIL_ALIGN_SIZE - 1)) ||
 	    (a.used_user_addr & (VRING_USED_ALIGN_SIZE - 1)) ||
 	    (a.log_guest_addr & (VRING_USED_ALIGN_SIZE - 1)))
@@ -2840,7 +2840,7 @@ void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
 }
 EXPORT_SYMBOL_GPL(vhost_signal);
 
-/* And here's the combo meal deal.  Supersize me! */
+/* Add to used ring and signal guest. */
 void vhost_add_used_and_signal(struct vhost_dev *dev,
 			       struct vhost_virtqueue *vq,
 			       unsigned int head, int len)
@@ -2860,7 +2860,7 @@ void vhost_add_used_and_signal_n(struct vhost_dev *dev,
 }
 EXPORT_SYMBOL_GPL(vhost_add_used_and_signal_n);
 
-/* return true if we're sure that avaiable ring is empty */
+/* return true if we're sure that available ring is empty */
 bool vhost_vq_avail_empty(struct vhost_dev *dev, struct vhost_virtqueue *vq)
 {
 	int r;
-- 
2.47.1

Re: [PATCH] vhost: Fix typos in comments and clarity on alignof usage

Posted by Michael S. Tsirkin 2 months, 3 weeks ago

On Sun, Jun 15, 2025 at 10:39:11AM -0700, Alok Tiwari wrote:
> This patch fixes multiple typos and improves comment clarity across
> vhost.c.
> - Correct spelling errors: "thead" -> "thread", "RUNNUNG" -> "RUNNING"
>   and "available".
> - Improve comment by replacing informal comment ("Supersize me!")
>   with a clear description.
> - Use __alignof__ correctly on dereferenced pointer types for better
>   readability and alignment with kernel documentation.
> 
> These changes enhance code readability and maintainability.
> 
> Signed-off-by: Alok Tiwari <alok.a.tiwari@oracle.com>

Thanks, I applied parts of it.

> ---
>  drivers/vhost/vhost.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 3a5ebb973dba..0227c123c0e0 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -594,10 +594,10 @@ static void vhost_attach_mm(struct vhost_dev *dev)
>  	if (dev->use_worker) {
>  		dev->mm = get_task_mm(current);
>  	} else {
> -		/* vDPA device does not use worker thead, so there's
> -		 * no need to hold the address space for mm. This help
> +		/* vDPA device does not use worker thread, so there's
> +		 * no need to hold the address space for mm. This helps
>  		 * to avoid deadlock in the case of mmap() which may
> -		 * held the refcnt of the file and depends on release
> +		 * hold the refcnt of the file and depends on release
>  		 * method to remove vma.
>  		 */
>  		dev->mm = current->mm;
> @@ -731,7 +731,7 @@ static void __vhost_vq_attach_worker(struct vhost_virtqueue *vq,
>  	 * We don't want to call synchronize_rcu for every vq during setup
>  	 * because it will slow down VM startup. If we haven't done
>  	 * VHOST_SET_VRING_KICK and not done the driver specific
> -	 * SET_ENDPOINT/RUNNUNG then we can skip the sync since there will
> +	 * SET_ENDPOINT/RUNNING then we can skip the sync since there will
>  	 * not be any works queued for scsi and net.
>  	 */
>  	mutex_lock(&vq->mutex);
> @@ -1898,8 +1898,8 @@ static long vhost_vring_set_addr(struct vhost_dev *d,
>  		return -EFAULT;
>  
>  	/* Make sure it's safe to cast pointers to vring types. */
> -	BUILD_BUG_ON(__alignof__ *vq->avail > VRING_AVAIL_ALIGN_SIZE);
> -	BUILD_BUG_ON(__alignof__ *vq->used > VRING_USED_ALIGN_SIZE);
> +	BUILD_BUG_ON(__alignof__(*vq->avail) > VRING_AVAIL_ALIGN_SIZE);
> +	BUILD_BUG_ON(__alignof__(*vq->used) > VRING_USED_ALIGN_SIZE);
>  	if ((a.avail_user_addr & (VRING_AVAIL_ALIGN_SIZE - 1)) ||
>  	    (a.used_user_addr & (VRING_USED_ALIGN_SIZE - 1)) ||
>  	    (a.log_guest_addr & (VRING_USED_ALIGN_SIZE - 1)))
> @@ -2840,7 +2840,7 @@ void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
>  }
>  EXPORT_SYMBOL_GPL(vhost_signal);
>  
> -/* And here's the combo meal deal.  Supersize me! */
> +/* Add to used ring and signal guest. */
>  void vhost_add_used_and_signal(struct vhost_dev *dev,
>  			       struct vhost_virtqueue *vq,
>  			       unsigned int head, int len)
> @@ -2860,7 +2860,7 @@ void vhost_add_used_and_signal_n(struct vhost_dev *dev,
>  }
>  EXPORT_SYMBOL_GPL(vhost_add_used_and_signal_n);
>  
> -/* return true if we're sure that avaiable ring is empty */
> +/* return true if we're sure that available ring is empty */
>  bool vhost_vq_avail_empty(struct vhost_dev *dev, struct vhost_virtqueue *vq)
>  {
>  	int r;
> -- 
> 2.47.1

Re: [PATCH] vhost: Fix typos in comments and clarity on alignof usage

Posted by Simon Horman 3 months, 3 weeks ago

On Sun, Jun 15, 2025 at 10:39:11AM -0700, Alok Tiwari wrote:
> This patch fixes multiple typos and improves comment clarity across
> vhost.c.
> - Correct spelling errors: "thead" -> "thread", "RUNNUNG" -> "RUNNING"
>   and "available".
> - Improve comment by replacing informal comment ("Supersize me!")
>   with a clear description.
> - Use __alignof__ correctly on dereferenced pointer types for better
>   readability and alignment with kernel documentation.

Could you expand on the last point?
I see that the patch uses __alignof__ with rather than without parentheses.
But I don't follow how that corresponds with the comment above.

> 
> These changes enhance code readability and maintainability.
> 
> Signed-off-by: Alok Tiwari <alok.a.tiwari@oracle.com>

...

> @@ -1898,8 +1898,8 @@ static long vhost_vring_set_addr(struct vhost_dev *d,
>  		return -EFAULT;
>  
>  	/* Make sure it's safe to cast pointers to vring types. */
> -	BUILD_BUG_ON(__alignof__ *vq->avail > VRING_AVAIL_ALIGN_SIZE);
> -	BUILD_BUG_ON(__alignof__ *vq->used > VRING_USED_ALIGN_SIZE);
> +	BUILD_BUG_ON(__alignof__(*vq->avail) > VRING_AVAIL_ALIGN_SIZE);
> +	BUILD_BUG_ON(__alignof__(*vq->used) > VRING_USED_ALIGN_SIZE);
>  	if ((a.avail_user_addr & (VRING_AVAIL_ALIGN_SIZE - 1)) ||
>  	    (a.used_user_addr & (VRING_USED_ALIGN_SIZE - 1)) ||
>  	    (a.log_guest_addr & (VRING_USED_ALIGN_SIZE - 1)))

...

Re: Re: [PATCH] vhost: Fix typos in comments and clarity on alignof usage

Posted by ALOK TIWARI 3 months, 3 weeks ago

Thanks Simon,

On 6/18/2025 12:07 AM, Simon Horman wrote:
> On Sun, Jun 15, 2025 at 10:39:11AM -0700, Alok Tiwari wrote:
>> This patch fixes multiple typos and improves comment clarity across
>> vhost.c.
>> - Correct spelling errors: "thead" -> "thread", "RUNNUNG" -> "RUNNING"
>>    and "available".
>> - Improve comment by replacing informal comment ("Supersize me!")
>>    with a clear description.
>> - Use __alignof__ correctly on dereferenced pointer types for better
>>    readability and alignment with kernel documentation.
> Could you expand on the last point?
> I see that the patch uses __alignof__ with rather than without parentheses.
> But I don't follow how that corresponds with the comment above.

only I can say "__alignof__ *vq->avail" is valid C,
but it can hard to read and easy to misinterpret.
Without proper parentheses sometime, __alignof__ *vq->avail can be 
misleading to reader. it may not be immediately clear whether it refers 
to alignment of the pointer vq->avail or
alignment of the object it points to.
__alignof__(*vq->avail) adds parentheses that clarify the intention 
explicitly.
I can not see very clear guide line to using parentheses or not for 
__alignof__ in kernel document apart 
from(https://www.kernel.org/doc/html/latest/process/coding-style.html).
Additionally, I have not been able to locate examples in the kernel code 
where __alignof__ is used without parentheses.

Thanks,
Alok

Re: Re: [PATCH] vhost: Fix typos in comments and clarity on alignof usage

Posted by Simon Horman 3 months, 3 weeks ago

On Wed, Jun 18, 2025 at 01:31:09AM +0530, ALOK TIWARI wrote:
> 
> 
> Thanks Simon,
> 
> On 6/18/2025 12:07 AM, Simon Horman wrote:
> > On Sun, Jun 15, 2025 at 10:39:11AM -0700, Alok Tiwari wrote:
> > > This patch fixes multiple typos and improves comment clarity across
> > > vhost.c.
> > > - Correct spelling errors: "thead" -> "thread", "RUNNUNG" -> "RUNNING"
> > >    and "available".
> > > - Improve comment by replacing informal comment ("Supersize me!")
> > >    with a clear description.
> > > - Use __alignof__ correctly on dereferenced pointer types for better
> > >    readability and alignment with kernel documentation.
> > Could you expand on the last point?
> > I see that the patch uses __alignof__ with rather than without parentheses.
> > But I don't follow how that corresponds with the comment above.
> 
> only I can say "__alignof__ *vq->avail" is valid C,
> but it can hard to read and easy to misinterpret.
> Without proper parentheses sometime, __alignof__ *vq->avail can be
> misleading to reader. it may not be immediately clear whether it refers to
> alignment of the pointer vq->avail or
> alignment of the object it points to.
> __alignof__(*vq->avail) adds parentheses that clarify the intention
> explicitly.
> I can not see very clear guide line to using parentheses or not for
> __alignof__ in kernel document apart
> from(https://www.kernel.org/doc/html/latest/process/coding-style.html).
> Additionally, I have not been able to locate examples in the kernel code
> where __alignof__ is used without parentheses.

Thanks, I understand now.

Perhaps it's not important, but FWIIW I was confused by "correctly".
And something like this seems a bit clearer to me.

  - Use __alignof__ with parentheses which is in keeping with kernel coding
    style for an __attribute__ and arguably improves readability of what is
    being aligned.

In any case, thanks for your explanation.
This patch now looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>