Re: [PATCH 2/2] x86/resctrl: Don't workqueue local event counter reads

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Tony,

On 11/5/24 3:39 PM, Luck, Tony wrote:
>> I think this change already undoes the motivation for 09909e098113
>> ("x86/resctrl: Queue mon_event_read() instead of sending an IPI")? As you mention in
>> changelog the goal of that work was to enable resctrl_arch_rmid_read() to sleep.
>> This change will call resctrl_arch_rmid_read() with preemption disabled if
>> it happens to be called on CPU in monitoring domain. Would that not cause
>> MPAM monitor count reads from CPU in domain to be a bug?
>>
>> Could you please try out this patch with CONFIG_DEBUG_ATOMIC_SLEEP=y?
> 
> How is this all going to look after the split into fs/resctrl and arch/* ?

Unclear to me at this point. Peter exposed an issue with current implementation
and this needs to be fixed. Since this involves preparatory work that impacts
systems currently supported we could also consider reverting to original behavior
and go back to drawing board with the preparatory work.

> Is the file system code going to have implementation choices that prevent
> performance sensitive users like Peter from optimizing monitor event
> reads by binding the monitor process to a CPU in the right domain
> to avoid IPI?

Apologies for not clearly stating it but I do agree that there is an issue
that needs to be fixed.

My response was not intended to be interpreted as a NACK but instead an attempt
to engage in discussion by pointing out that the proposed fix may not be ideal.

I tried out my own suggestion and indeed when just trying to mount resctrl
on x86 with this patch applied results in:
	BUG: scheduling while atomic

I do not object to optimizing monitor event reads but the proposed fix
is not appropriate in its current form.

Reinette