[PATCH 1/2] virtio_pci: Don't make an extra copy of cpu affinity mask

[PATCH 1/2] virtio_pci: Don't make an extra copy of cpu affinity mask

[Jakub Sitnicki via Virtualization]

Since commit 19e226e8cc5d ("virtio: Make vp_set_vq_affinity() take a
mask.") it is actually not needed to have a local copy of the cpu mask.

Pass the cpu mask we got as argument to set the irq affinity hint.

Cc: Caleb Raitto <caraitto@google.com>
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 drivers/virtio/virtio_pci_common.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index c2524a7207cf..8927bc338f06 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -433,21 +433,14 @@ int vp_set_vq_affinity(struct virtqueue *vq, const struct cpumask *cpu_mask)
 	struct virtio_device *vdev = vq->vdev;
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct virtio_pci_vq_info *info = vp_dev->vqs[vq->index];
-	struct cpumask *mask;
 	unsigned int irq;
 
 	if (!vq->callback)
 		return -EINVAL;
 
 	if (vp_dev->msix_enabled) {
-		mask = vp_dev->msix_affinity_masks[info->msix_vector];
 		irq = pci_irq_vector(vp_dev->pci_dev, info->msix_vector);
-		if (!cpu_mask)
-			irq_set_affinity_hint(irq, NULL);
-		else {
-			cpumask_copy(mask, cpu_mask);
-			irq_set_affinity_hint(irq, mask);
-		}
+		irq_set_affinity_hint(irq, cpu_mask);
 	}
 	return 0;
 }
-- 
2.41.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 2/2] virtio_pci: Switch away from deprecated irq_set_affinity_hint

[Jakub Sitnicki via Virtualization]

Since commit 65c7cdedeb30 ("genirq: Provide new interfaces for affinity
hints") irq_set_affinity_hint is being phased out.

Switch to new interfaces for setting and applying irq affinity hints.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 drivers/virtio/virtio_pci_common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 8927bc338f06..9fab99b540f1 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -242,7 +242,7 @@ void vp_del_vqs(struct virtio_device *vdev)
 			if (v != VIRTIO_MSI_NO_VECTOR) {
 				int irq = pci_irq_vector(vp_dev->pci_dev, v);
 
-				irq_set_affinity_hint(irq, NULL);
+				irq_update_affinity_hint(irq, NULL);
 				free_irq(irq, vq);
 			}
 		}
@@ -440,7 +440,7 @@ int vp_set_vq_affinity(struct virtqueue *vq, const struct cpumask *cpu_mask)
 
 	if (vp_dev->msix_enabled) {
 		irq = pci_irq_vector(vp_dev->pci_dev, info->msix_vector);
-		irq_set_affinity_hint(irq, cpu_mask);
+		irq_set_affinity_and_hint(irq, cpu_mask);
 	}
 	return 0;
 }
-- 
2.41.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/2] virtio_pci: Switch away from deprecated irq_set_affinity_hint

[Xuan Zhuo]

On Thu, 19 Oct 2023 12:16:25 +0200, Jakub Sitnicki <jakub@cloudflare.com> wrote:
> Since commit 65c7cdedeb30 ("genirq: Provide new interfaces for affinity
> hints") irq_set_affinity_hint is being phased out.
>
> Switch to new interfaces for setting and applying irq affinity hints.
>
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>

Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

> ---
>  drivers/virtio/virtio_pci_common.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 8927bc338f06..9fab99b540f1 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -242,7 +242,7 @@ void vp_del_vqs(struct virtio_device *vdev)
>  			if (v != VIRTIO_MSI_NO_VECTOR) {
>  				int irq = pci_irq_vector(vp_dev->pci_dev, v);
>
> -				irq_set_affinity_hint(irq, NULL);
> +				irq_update_affinity_hint(irq, NULL);
>  				free_irq(irq, vq);
>  			}
>  		}
> @@ -440,7 +440,7 @@ int vp_set_vq_affinity(struct virtqueue *vq, const struct cpumask *cpu_mask)
>
>  	if (vp_dev->msix_enabled) {
>  		irq = pci_irq_vector(vp_dev->pci_dev, info->msix_vector);
> -		irq_set_affinity_hint(irq, cpu_mask);
> +		irq_set_affinity_and_hint(irq, cpu_mask);
>  	}
>  	return 0;
>  }
> --
> 2.41.0
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] virtio_pci: Don't make an extra copy of cpu affinity mask

[Xuan Zhuo]

On Thu, 19 Oct 2023 12:16:24 +0200, Jakub Sitnicki <jakub@cloudflare.com> wrote:
> Since commit 19e226e8cc5d ("virtio: Make vp_set_vq_affinity() take a
> mask.") it is actually not needed to have a local copy of the cpu mask.


Could you give more info to prove this?

If you are right, I think you should delete all code about msix_affinity_masks?

Thanks.

>
> Pass the cpu mask we got as argument to set the irq affinity hint.
>
> Cc: Caleb Raitto <caraitto@google.com>
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---
>  drivers/virtio/virtio_pci_common.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index c2524a7207cf..8927bc338f06 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -433,21 +433,14 @@ int vp_set_vq_affinity(struct virtqueue *vq, const struct cpumask *cpu_mask)
>  	struct virtio_device *vdev = vq->vdev;
>  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>  	struct virtio_pci_vq_info *info = vp_dev->vqs[vq->index];
> -	struct cpumask *mask;
>  	unsigned int irq;
>
>  	if (!vq->callback)
>  		return -EINVAL;
>
>  	if (vp_dev->msix_enabled) {
> -		mask = vp_dev->msix_affinity_masks[info->msix_vector];
>  		irq = pci_irq_vector(vp_dev->pci_dev, info->msix_vector);
> -		if (!cpu_mask)
> -			irq_set_affinity_hint(irq, NULL);
> -		else {
> -			cpumask_copy(mask, cpu_mask);
> -			irq_set_affinity_hint(irq, mask);
> -		}
> +		irq_set_affinity_hint(irq, cpu_mask);
>  	}
>  	return 0;
>  }
> --
> 2.41.0
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] virtio_pci: Don't make an extra copy of cpu affinity mask

[Jakub Sitnicki via Virtualization]

On Thu, Oct 19, 2023 at 08:55 PM +08, Xuan Zhuo wrote:
> On Thu, 19 Oct 2023 12:16:24 +0200, Jakub Sitnicki <jakub@cloudflare.com> wrote:
>> Since commit 19e226e8cc5d ("virtio: Make vp_set_vq_affinity() take a
>> mask.") it is actually not needed to have a local copy of the cpu mask.
>
>
> Could you give more info to prove this?
>
> If you are right, I think you should delete all code about msix_affinity_masks?

Sorry for the late reply. I've been away.

It looks that msix_affinity_masks became unused - intentionally - in
2015, after commit 210d150e1f5d ("virtio_pci: Clear stale cpumask when
setting irq affinity") [1].

Originally introduced in 2012 in commit 75a0a52be3c2 ("virtio: introduce
an API to set affinity for a virtqueue") [2]. As I understand, it was
meant to make it possible to set VQ affinity to more than once CPU.

Now that we can pass a CPU mask, listing all CPUs, to set_vq_affinity,
msix_affinity_masks seems to no longer have a purpose.

So, IMO, you're right. We can remove it.

Happy to do that in a follow up series.

That is - if you're okay with these two patches in the current form.

Thanks for reviewing.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=210d150e1f5da506875e376422ba31ead2d49621
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=75a0a52be3c27b58654fbed2c8f2ff401482b9a4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] virtio_pci: Don't make an extra copy of cpu affinity mask

[Xuan Zhuo]

On Mon, 23 Oct 2023 18:52:45 +0200, Jakub Sitnicki <jakub@cloudflare.com> wrote:
> On Thu, Oct 19, 2023 at 08:55 PM +08, Xuan Zhuo wrote:
> > On Thu, 19 Oct 2023 12:16:24 +0200, Jakub Sitnicki <jakub@cloudflare.com> wrote:
> >> Since commit 19e226e8cc5d ("virtio: Make vp_set_vq_affinity() take a
> >> mask.") it is actually not needed to have a local copy of the cpu mask.
> >
> >
> > Could you give more info to prove this?


Actually, my question is that can we pass a val on the stack(or temp value) to
the irq_set_affinity_hint()?

Such as the virtio-net uses zalloc_cpumask_var to alloc a cpu_mask, and
that will be released.



	int __irq_apply_affinity_hint(unsigned int irq, const struct cpumask *m,
				      bool setaffinity)
	{
		unsigned long flags;
		struct irq_desc *desc = irq_get_desc_lock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);

		if (!desc)
			return -EINVAL;
->		desc->affinity_hint = m;
		irq_put_desc_unlock(desc, flags);
		if (m && setaffinity)
			__irq_set_affinity(irq, m, false);
		return 0;
	}
	EXPORT_SYMBOL_GPL(__irq_apply_affinity_hint);

The above code directly refers the mask pointer. If the mask is a temp value, I
think that is a bug.

And I notice that many places directly pass the temp value to this API.
And I am a little confused. ^_^ Or I missed something.

Thanks.


> >
> > If you are right, I think you should delete all code about msix_affinity_masks?
>
> Sorry for the late reply. I've been away.
>
> It looks that msix_affinity_masks became unused - intentionally - in
> 2015, after commit 210d150e1f5d ("virtio_pci: Clear stale cpumask when
> setting irq affinity") [1].
>
> Originally introduced in 2012 in commit 75a0a52be3c2 ("virtio: introduce
> an API to set affinity for a virtqueue") [2]. As I understand, it was
> meant to make it possible to set VQ affinity to more than once CPU.
>
> Now that we can pass a CPU mask, listing all CPUs, to set_vq_affinity,
> msix_affinity_masks seems to no longer have a purpose.
>
> So, IMO, you're right. We can remove it.
>
> Happy to do that in a follow up series.
>
> That is - if you're okay with these two patches in the current form.
>
> Thanks for reviewing.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=210d150e1f5da506875e376422ba31ead2d49621
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=75a0a52be3c27b58654fbed2c8f2ff401482b9a4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] virtio_pci: Don't make an extra copy of cpu affinity mask

[Jakub Sitnicki via Virtualization]

On Tue, Oct 24, 2023 at 10:31 AM +08, Xuan Zhuo wrote:
> On Mon, 23 Oct 2023 18:52:45 +0200, Jakub Sitnicki <jakub@cloudflare.com> wrote:
>> On Thu, Oct 19, 2023 at 08:55 PM +08, Xuan Zhuo wrote:
>> > On Thu, 19 Oct 2023 12:16:24 +0200, Jakub Sitnicki <jakub@cloudflare.com> wrote:
>> >> Since commit 19e226e8cc5d ("virtio: Make vp_set_vq_affinity() take a
>> >> mask.") it is actually not needed to have a local copy of the cpu mask.
>> >
>> >
>> > Could you give more info to prove this?
>
>
> Actually, my question is that can we pass a val on the stack(or temp value) to
> the irq_set_affinity_hint()?
>
> Such as the virtio-net uses zalloc_cpumask_var to alloc a cpu_mask, and
> that will be released.
>
>
>
> 	int __irq_apply_affinity_hint(unsigned int irq, const struct cpumask *m,
> 				      bool setaffinity)
> 	{
> 		unsigned long flags;
> 		struct irq_desc *desc = irq_get_desc_lock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
>
> 		if (!desc)
> 			return -EINVAL;
> ->		desc->affinity_hint = m;
> 		irq_put_desc_unlock(desc, flags);
> 		if (m && setaffinity)
> 			__irq_set_affinity(irq, m, false);
> 		return 0;
> 	}
> 	EXPORT_SYMBOL_GPL(__irq_apply_affinity_hint);
>
> The above code directly refers the mask pointer. If the mask is a temp value, I
> think that is a bug.

You are completely right. irq_set_affinity_hint stores the mask pointer.
irq_affinity_hint_proc_show later dereferences it when user reads out
/proc/irq/*/affinity_hint.

I have failed to notice that. That's why we need cpumask_copy to stay.

My patch is buggy. Please disregard.

I will send a v2 to only migrate from deprecated irq_set_affinity_hint.

> And I notice that many places directly pass the temp value to this API.
> And I am a little confused. ^_^ Or I missed something.

There seem two be two gropus of callers:

1. Those that use get_cpu_mask/cpumask_of/cpumask_of_node to produce a
   cpumask pointer which is a preallocated constant.

   $ weggli 'irq_set_affinity_hint(_, $func(_));' ~/src/linux

2. Those that pass a pointer to memory somewhere.

   $ weggli 'irq_set_affinity_hint(_, $mask);' ~/src/linux

(weggli tool can be found at https://github.com/weggli-rs/weggli)

I've looked over the callers from group #2 but I couldn't find any
passing a pointer memory on stack :-)

Thanks for pointing this out.

[...]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] virtio_pci: Don't make an extra copy of cpu affinity mask

[Xuan Zhuo]

On Tue, 24 Oct 2023 10:17:19 +0200, Jakub Sitnicki <jakub@cloudflare.com> wrote:
> On Tue, Oct 24, 2023 at 10:31 AM +08, Xuan Zhuo wrote:
> > On Mon, 23 Oct 2023 18:52:45 +0200, Jakub Sitnicki <jakub@cloudflare.com> wrote:
> >> On Thu, Oct 19, 2023 at 08:55 PM +08, Xuan Zhuo wrote:
> >> > On Thu, 19 Oct 2023 12:16:24 +0200, Jakub Sitnicki <jakub@cloudflare.com> wrote:
> >> >> Since commit 19e226e8cc5d ("virtio: Make vp_set_vq_affinity() take a
> >> >> mask.") it is actually not needed to have a local copy of the cpu mask.
> >> >
> >> >
> >> > Could you give more info to prove this?
> >
> >
> > Actually, my question is that can we pass a val on the stack(or temp value) to
> > the irq_set_affinity_hint()?
> >
> > Such as the virtio-net uses zalloc_cpumask_var to alloc a cpu_mask, and
> > that will be released.
> >
> >
> >
> > 	int __irq_apply_affinity_hint(unsigned int irq, const struct cpumask *m,
> > 				      bool setaffinity)
> > 	{
> > 		unsigned long flags;
> > 		struct irq_desc *desc = irq_get_desc_lock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
> >
> > 		if (!desc)
> > 			return -EINVAL;
> > ->		desc->affinity_hint = m;
> > 		irq_put_desc_unlock(desc, flags);
> > 		if (m && setaffinity)
> > 			__irq_set_affinity(irq, m, false);
> > 		return 0;
> > 	}
> > 	EXPORT_SYMBOL_GPL(__irq_apply_affinity_hint);
> >
> > The above code directly refers the mask pointer. If the mask is a temp value, I
> > think that is a bug.
>
> You are completely right. irq_set_affinity_hint stores the mask pointer.
> irq_affinity_hint_proc_show later dereferences it when user reads out
> /proc/irq/*/affinity_hint.
>
> I have failed to notice that. That's why we need cpumask_copy to stay.
>
> My patch is buggy. Please disregard.
>
> I will send a v2 to only migrate from deprecated irq_set_affinity_hint.
>
> > And I notice that many places directly pass the temp value to this API.
> > And I am a little confused. ^_^ Or I missed something.
>
> There seem two be two gropus of callers:
>
> 1. Those that use get_cpu_mask/cpumask_of/cpumask_of_node to produce a
>    cpumask pointer which is a preallocated constant.
>
>    $ weggli 'irq_set_affinity_hint(_, $func(_));' ~/src/linux
>
> 2. Those that pass a pointer to memory somewhere.
>
>    $ weggli 'irq_set_affinity_hint(_, $mask);' ~/src/linux
>
> (weggli tool can be found at https://github.com/weggli-rs/weggli)
>
> I've looked over the callers from group #2 but I couldn't find any
> passing a pointer memory on stack :-)

Pls check stmmac_request_irq_multi_msi()

Thanks.


>
> Thanks for pointing this out.
>
> [...]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] virtio_pci: Don't make an extra copy of cpu affinity mask

[Jakub Sitnicki via Virtualization]

On Tue, Oct 24, 2023 at 06:53 PM +08, Xuan Zhuo wrote:
> On Tue, 24 Oct 2023 10:17:19 +0200, Jakub Sitnicki <jakub@cloudflare.com> wrote:
>> On Tue, Oct 24, 2023 at 10:31 AM +08, Xuan Zhuo wrote:
>> > On Mon, 23 Oct 2023 18:52:45 +0200, Jakub Sitnicki <jakub@cloudflare.com> wrote:
>> >> On Thu, Oct 19, 2023 at 08:55 PM +08, Xuan Zhuo wrote:
>> >> > On Thu, 19 Oct 2023 12:16:24 +0200, Jakub Sitnicki <jakub@cloudflare.com> wrote:
>> >> >> Since commit 19e226e8cc5d ("virtio: Make vp_set_vq_affinity() take a
>> >> >> mask.") it is actually not needed to have a local copy of the cpu mask.
>> >> >
>> >> >
>> >> > Could you give more info to prove this?
>> >
>> >
>> > Actually, my question is that can we pass a val on the stack(or temp value) to
>> > the irq_set_affinity_hint()?
>> >
>> > Such as the virtio-net uses zalloc_cpumask_var to alloc a cpu_mask, and
>> > that will be released.
>> >
>> >
>> >
>> > 	int __irq_apply_affinity_hint(unsigned int irq, const struct cpumask *m,
>> > 				      bool setaffinity)
>> > 	{
>> > 		unsigned long flags;
>> > 		struct irq_desc *desc = irq_get_desc_lock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
>> >
>> > 		if (!desc)
>> > 			return -EINVAL;
>> > ->		desc->affinity_hint = m;
>> > 		irq_put_desc_unlock(desc, flags);
>> > 		if (m && setaffinity)
>> > 			__irq_set_affinity(irq, m, false);
>> > 		return 0;
>> > 	}
>> > 	EXPORT_SYMBOL_GPL(__irq_apply_affinity_hint);
>> >
>> > The above code directly refers the mask pointer. If the mask is a temp value, I
>> > think that is a bug.
>>
>> You are completely right. irq_set_affinity_hint stores the mask pointer.
>> irq_affinity_hint_proc_show later dereferences it when user reads out
>> /proc/irq/*/affinity_hint.
>>
>> I have failed to notice that. That's why we need cpumask_copy to stay.
>>
>> My patch is buggy. Please disregard.
>>
>> I will send a v2 to only migrate from deprecated irq_set_affinity_hint.
>>
>> > And I notice that many places directly pass the temp value to this API.
>> > And I am a little confused. ^_^ Or I missed something.
>>
>> There seem two be two gropus of callers:
>>
>> 1. Those that use get_cpu_mask/cpumask_of/cpumask_of_node to produce a
>>    cpumask pointer which is a preallocated constant.
>>
>>    $ weggli 'irq_set_affinity_hint(_, $func(_));' ~/src/linux
>>
>> 2. Those that pass a pointer to memory somewhere.
>>
>>    $ weggli 'irq_set_affinity_hint(_, $mask);' ~/src/linux
>>
>> (weggli tool can be found at https://github.com/weggli-rs/weggli)
>>
>> I've looked over the callers from group #2 but I couldn't find any
>> passing a pointer memory on stack :-)
>
> Pls check stmmac_request_irq_multi_msi()

Good catch. That one looks buggy.

I should also checked for callers that take an address of a var/field:

  $ weggli 'irq_set_affinity_hint(_, &$mask);' ~/src/linux
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] virtio_pci: Don't make an extra copy of cpu affinity mask

[Xuan Zhuo]

On Tue, 24 Oct 2023 13:26:49 +0200, Jakub Sitnicki <jakub@cloudflare.com> wrote:
> On Tue, Oct 24, 2023 at 06:53 PM +08, Xuan Zhuo wrote:
> > On Tue, 24 Oct 2023 10:17:19 +0200, Jakub Sitnicki <jakub@cloudflare.com> wrote:
> >> On Tue, Oct 24, 2023 at 10:31 AM +08, Xuan Zhuo wrote:
> >> > On Mon, 23 Oct 2023 18:52:45 +0200, Jakub Sitnicki <jakub@cloudflare.com> wrote:
> >> >> On Thu, Oct 19, 2023 at 08:55 PM +08, Xuan Zhuo wrote:
> >> >> > On Thu, 19 Oct 2023 12:16:24 +0200, Jakub Sitnicki <jakub@cloudflare.com> wrote:
> >> >> >> Since commit 19e226e8cc5d ("virtio: Make vp_set_vq_affinity() take a
> >> >> >> mask.") it is actually not needed to have a local copy of the cpu mask.
> >> >> >
> >> >> >
> >> >> > Could you give more info to prove this?
> >> >
> >> >
> >> > Actually, my question is that can we pass a val on the stack(or temp value) to
> >> > the irq_set_affinity_hint()?
> >> >
> >> > Such as the virtio-net uses zalloc_cpumask_var to alloc a cpu_mask, and
> >> > that will be released.
> >> >
> >> >
> >> >
> >> > 	int __irq_apply_affinity_hint(unsigned int irq, const struct cpumask *m,
> >> > 				      bool setaffinity)
> >> > 	{
> >> > 		unsigned long flags;
> >> > 		struct irq_desc *desc = irq_get_desc_lock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
> >> >
> >> > 		if (!desc)
> >> > 			return -EINVAL;
> >> > ->		desc->affinity_hint = m;
> >> > 		irq_put_desc_unlock(desc, flags);
> >> > 		if (m && setaffinity)
> >> > 			__irq_set_affinity(irq, m, false);
> >> > 		return 0;
> >> > 	}
> >> > 	EXPORT_SYMBOL_GPL(__irq_apply_affinity_hint);
> >> >
> >> > The above code directly refers the mask pointer. If the mask is a temp value, I
> >> > think that is a bug.
> >>
> >> You are completely right. irq_set_affinity_hint stores the mask pointer.
> >> irq_affinity_hint_proc_show later dereferences it when user reads out
> >> /proc/irq/*/affinity_hint.
> >>
> >> I have failed to notice that. That's why we need cpumask_copy to stay.
> >>
> >> My patch is buggy. Please disregard.
> >>
> >> I will send a v2 to only migrate from deprecated irq_set_affinity_hint.
> >>
> >> > And I notice that many places directly pass the temp value to this API.
> >> > And I am a little confused. ^_^ Or I missed something.
> >>
> >> There seem two be two gropus of callers:
> >>
> >> 1. Those that use get_cpu_mask/cpumask_of/cpumask_of_node to produce a
> >>    cpumask pointer which is a preallocated constant.
> >>
> >>    $ weggli 'irq_set_affinity_hint(_, $func(_));' ~/src/linux
> >>
> >> 2. Those that pass a pointer to memory somewhere.
> >>
> >>    $ weggli 'irq_set_affinity_hint(_, $mask);' ~/src/linux
> >>
> >> (weggli tool can be found at https://github.com/weggli-rs/weggli)
> >>
> >> I've looked over the callers from group #2 but I couldn't find any
> >> passing a pointer memory on stack :-)
> >
> > Pls check stmmac_request_irq_multi_msi()
>
> Good catch. That one looks buggy.
>
> I should also checked for callers that take an address of a var/field:
>
>   $ weggli 'irq_set_affinity_hint(_, &$mask);' ~/src/linux

Do you find more?

Thanks.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] virtio_pci: Don't make an extra copy of cpu affinity mask

[Jakub Sitnicki via Virtualization]

On Tue, Oct 24, 2023 at 07:46 PM +08, Xuan Zhuo wrote:
> On Tue, 24 Oct 2023 13:26:49 +0200, Jakub Sitnicki <jakub@cloudflare.com> wrote:
>> On Tue, Oct 24, 2023 at 06:53 PM +08, Xuan Zhuo wrote:
>> > On Tue, 24 Oct 2023 10:17:19 +0200, Jakub Sitnicki <jakub@cloudflare.com> wrote:
>> >> On Tue, Oct 24, 2023 at 10:31 AM +08, Xuan Zhuo wrote:
>> >> > On Mon, 23 Oct 2023 18:52:45 +0200, Jakub Sitnicki <jakub@cloudflare.com> wrote:
>> >> >> On Thu, Oct 19, 2023 at 08:55 PM +08, Xuan Zhuo wrote:
>> >> >> > On Thu, 19 Oct 2023 12:16:24 +0200, Jakub Sitnicki <jakub@cloudflare.com> wrote:
>> >> >> >> Since commit 19e226e8cc5d ("virtio: Make vp_set_vq_affinity() take a
>> >> >> >> mask.") it is actually not needed to have a local copy of the cpu mask.
>> >> >> >
>> >> >> >
>> >> >> > Could you give more info to prove this?
>> >> >
>> >> >
>> >> > Actually, my question is that can we pass a val on the stack(or temp value) to
>> >> > the irq_set_affinity_hint()?
>> >> >
>> >> > Such as the virtio-net uses zalloc_cpumask_var to alloc a cpu_mask, and
>> >> > that will be released.
>> >> >
>> >> >
>> >> >
>> >> > 	int __irq_apply_affinity_hint(unsigned int irq, const struct cpumask *m,
>> >> > 				      bool setaffinity)
>> >> > 	{
>> >> > 		unsigned long flags;
>> >> > 		struct irq_desc *desc = irq_get_desc_lock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
>> >> >
>> >> > 		if (!desc)
>> >> > 			return -EINVAL;
>> >> > ->		desc->affinity_hint = m;
>> >> > 		irq_put_desc_unlock(desc, flags);
>> >> > 		if (m && setaffinity)
>> >> > 			__irq_set_affinity(irq, m, false);
>> >> > 		return 0;
>> >> > 	}
>> >> > 	EXPORT_SYMBOL_GPL(__irq_apply_affinity_hint);
>> >> >
>> >> > The above code directly refers the mask pointer. If the mask is a temp value, I
>> >> > think that is a bug.
>> >>
>> >> You are completely right. irq_set_affinity_hint stores the mask pointer.
>> >> irq_affinity_hint_proc_show later dereferences it when user reads out
>> >> /proc/irq/*/affinity_hint.
>> >>
>> >> I have failed to notice that. That's why we need cpumask_copy to stay.
>> >>
>> >> My patch is buggy. Please disregard.
>> >>
>> >> I will send a v2 to only migrate from deprecated irq_set_affinity_hint.
>> >>
>> >> > And I notice that many places directly pass the temp value to this API.
>> >> > And I am a little confused. ^_^ Or I missed something.
>> >>
>> >> There seem two be two gropus of callers:
>> >>
>> >> 1. Those that use get_cpu_mask/cpumask_of/cpumask_of_node to produce a
>> >>    cpumask pointer which is a preallocated constant.
>> >>
>> >>    $ weggli 'irq_set_affinity_hint(_, $func(_));' ~/src/linux
>> >>
>> >> 2. Those that pass a pointer to memory somewhere.
>> >>
>> >>    $ weggli 'irq_set_affinity_hint(_, $mask);' ~/src/linux
>> >>
>> >> (weggli tool can be found at https://github.com/weggli-rs/weggli)
>> >>
>> >> I've looked over the callers from group #2 but I couldn't find any
>> >> passing a pointer memory on stack :-)
>> >
>> > Pls check stmmac_request_irq_multi_msi()
>>
>> Good catch. That one looks buggy.
>>
>> I should also checked for callers that take an address of a var/field:
>>
>>   $ weggli 'irq_set_affinity_hint(_, &$mask);' ~/src/linux
>
> Do you find more?

No, just the one you pointed out. Unless I missed something.

I ran an improved query. Shows everything but the non-interesting cases:

$ weggli '{
	NOT: irq_set_affinity_hint(_, NULL);
	NOT: irq_set_affinity_hint(_, get_cpu_mask(_));
	NOT: irq_set_affinity_hint(_, cpumask_of(_));
	irq_set_affinity_hint(_, _);
}' ~/src/linux

And repeated it for irq_set_affinity_and_hint and irq_update_affinity.

The calls where it was not obvious at first sight that we're passing a
pointer to some heap memory, turned out to use a temporary variable to
either store address to heap memory or return value from cpumask_of*().

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization