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 <[email protected]>
Signed-off-by: Jakub Sitnicki <[email protected]>
---
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
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 <[email protected]>
---
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
On Thu, 19 Oct 2023 12:16:25 +0200, Jakub Sitnicki <[email protected]> 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 <[email protected]>
Reviewed-by: Xuan Zhuo <[email protected]>
> ---
> 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
>
On Thu, 19 Oct 2023 12:16:24 +0200, Jakub Sitnicki <[email protected]> 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 <[email protected]>
> Signed-off-by: Jakub Sitnicki <[email protected]>
> ---
> 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
>
On Thu, Oct 19, 2023 at 08:55 PM +08, Xuan Zhuo wrote:
> On Thu, 19 Oct 2023 12:16:24 +0200, Jakub Sitnicki <[email protected]> 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
On Mon, 23 Oct 2023 18:52:45 +0200, Jakub Sitnicki <[email protected]> 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 <[email protected]> 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
On Tue, Oct 24, 2023 at 10:31 AM +08, Xuan Zhuo wrote:
> On Mon, 23 Oct 2023 18:52:45 +0200, Jakub Sitnicki <[email protected]> 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 <[email protected]> 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.
[...]
On Tue, 24 Oct 2023 10:17:19 +0200, Jakub Sitnicki <[email protected]> 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 <[email protected]> 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 <[email protected]> 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.
>
> [...]
On Tue, Oct 24, 2023 at 06:53 PM +08, Xuan Zhuo wrote:
> On Tue, 24 Oct 2023 10:17:19 +0200, Jakub Sitnicki <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
On Tue, 24 Oct 2023 13:26:49 +0200, Jakub Sitnicki <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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.
On Tue, Oct 24, 2023 at 07:46 PM +08, Xuan Zhuo wrote:
> On Tue, 24 Oct 2023 13:26:49 +0200, Jakub Sitnicki <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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*().