Messages in this thread | | | From | Jakub Sitnicki <> | Subject | Re: [PATCH] genirq: Own affinity hint | Date | Thu, 26 Oct 2023 15:05:26 +0200 |
| |
On Wed, Oct 25, 2023 at 10:10 PM +02, Thomas Gleixner wrote: > On Wed, Oct 25 2023 at 16:15, Jakub Sitnicki wrote: >> @@ -55,26 +55,33 @@ static int alloc_masks(struct irq_desc *desc, int node) >> { >> if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity, >> GFP_KERNEL, node)) >> - return -ENOMEM; >> + goto err_affinity; >> + if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity_hint, >> + GFP_KERNEL, node)) > > This gets allocated for every interrupt descriptor but only a few or > even none will ever use it. Seriously no.
Makes sense. I wanted to start the simplest possible approach first. I expect allocating it lazily will be more involved - have to cover the not-allocated case.
But thinking about it some more - perhaps what makes more sense is, for irq_set_affinity[_and]_hint users who don't want to bother with managing a cpumask on their side, to switch to irq_set_affinity.
That interface doesn't require the cpumask to outlive the call, AFAICT.
After all, setting the affinity hint only buys you an ability to read it out from /proc/irq/<N>/affinity_hint. Same information is available from /proc/irq/<N>/smp_affinity. Please set me straight, if I'm missing something here.
> >> + goto err_affinity_hint; >> >> #ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK >> if (!zalloc_cpumask_var_node(&desc->irq_common_data.effective_affinity, >> - GFP_KERNEL, node)) { >> - free_cpumask_var(desc->irq_common_data.affinity); >> - return -ENOMEM; >> - } >> + GFP_KERNEL, node)) >> + goto err_effective_affinity; >> #endif >> >> #ifdef CONFIG_GENERIC_PENDING_IRQ >> - if (!zalloc_cpumask_var_node(&desc->pending_mask, GFP_KERNEL, node)) { >> -#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK >> - free_cpumask_var(desc->irq_common_data.effective_affinity); >> -#endif >> - free_cpumask_var(desc->irq_common_data.affinity); >> - return -ENOMEM; >> - } >> + if (!zalloc_cpumask_var_node(&desc->pending_mask, GFP_KERNEL, node)) >> + goto err_pending_mask; >> #endif >> return 0; >> + >> +err_pending_mask: > > How is this supposed to compile with CONFIG_GENERIC_PENDING_IRQ=n ? > >> +#ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK >> + free_cpumask_var(desc->irq_common_data.effective_affinity); >> +#endif >> +err_effective_affinity: > > and this with CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK=n ?
If it wasn't for the unused label (my bad) both cases LGTM.
But I double checked. If I force feed it through the preprocessor:
* CONFIG_GENERIC_PENDING_IRQ=n :
static int alloc_masks(struct irq_desc *desc, int node) { if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity, ((( gfp_t)(0x400u|0x800u)) | (( gfp_t)0x40u) | (( gfp_t)0x80u)), node)) goto err_affinity; if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity_hint, ((( gfp_t)(0x400u|0x800u)) | (( gfp_t)0x40u) | (( gfp_t)0x80u)), node)) goto err_affinity_hint; if (!zalloc_cpumask_var_node(&desc->irq_common_data.effective_affinity, ((( gfp_t)(0x400u|0x800u)) | (( gfp_t)0x40u) | (( gfp_t)0x80u)), node)) goto err_effective_affinity; return 0; err_pending_mask: free_cpumask_var(desc->irq_common_data.effective_affinity); err_effective_affinity: free_cpumask_var(desc->irq_common_data.affinity_hint); err_affinity_hint: free_cpumask_var(desc->irq_common_data.affinity); err_affinity: return -12; }
* CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK=n :
static int alloc_masks(struct irq_desc *desc, int node) { if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity, ((( gfp_t)(0x400u|0x800u)) | (( gfp_t)0x40u) | (( gfp_t)0x80u)), node)) goto err_affinity; if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity_hint, ((( gfp_t)(0x400u|0x800u)) | (( gfp_t)0x40u) | (( gfp_t)0x80u)), node)) goto err_affinity_hint; if (!zalloc_cpumask_var_node(&desc->pending_mask, ((( gfp_t)(0x400u|0x800u)) | (( gfp_t)0x40u) | (( gfp_t)0x80u)), node)) goto err_pending_mask; return 0; err_pending_mask: err_effective_affinity: free_cpumask_var(desc->irq_common_data.affinity_hint); err_affinity_hint: free_cpumask_var(desc->irq_common_data.affinity); err_affinity: return -12; }
I did forget, however, to clean up the "old" affinity_hint field from irq_desc struct.
Anyway - not planning on sending a v2, unless you see value in doing a lazy allocation of the hint from within irq subsys.
Thanks for feedback.
| |