Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
References: <20231025141517.375378-1-jakub@cloudflare.com>
 <878r7q5upq.ffs@tglx>
User-agent: mu4e 1.6.10; emacs 28.3
From:   Jakub Sitnicki <jakub@cloudflare.com>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     linux-kernel@vger.kernel.org, kernel-team@cloudflare.com,
        Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Subject: Re: [PATCH] genirq: Own affinity hint
Date:   Thu, 26 Oct 2023 15:05:26 +0200
In-reply-to: <878r7q5upq.ffs@tglx>
Message-ID: <871qdh32vp.fsf@cloudflare.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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.