Hi,
On Tue, Dec 03, 2024 at 04:12:33PM +0000, Mark Brown wrote:
> On Tue, Dec 03, 2024 at 03:33:18PM +0000, Dave Martin wrote:
> > On Tue, Dec 03, 2024 at 12:45:57PM +0000, Mark Brown wrote:
>
> > > + get_cpu_fpsimd_context();
>
> > > + if (current->thread.svcr & SVCR_SM_MASK) {
> > > + memset(¤t->thread.uw.fpsimd_state.vregs, 0,
> > > + sizeof(current->thread.uw.fpsimd_state.vregs));
>
> > Do we need to hold the CPU fpsimd context across this memset?
>
> > IIRC, TIF_FOREIGN_FPSTATE can be spontaneously cleared along with
> > dumping of the regs into thread_struct (from current's PoV), but never
> > spontaneously set again. So ... -> [*]
>
> Yes, we could drop the lock here. OTOH this is very simple and easy to
> understand.
Ack; it works either way.
Since this is a Fixes: patch, it may be better to keep it simple.
>
> > > + /* Ensure any copies on other CPUs aren't reused */
> > > + fpsimd_flush_task_state(current);
>
> > (This is very similar to fpsimd_flush_thread(); can they be unified?)
>
> I have a half finished series to replace the whole setup around
> accessing the state with get/put operations for working on the state
> which should remove all these functions. The pile of similarly and
> confusingly named operations we have for working on the state is one of
> the major sources of issues with this code, even when actively working
> on the code it's hard to remember exactly which operation does what
> never mind the rules for which is needed.
Sure, something like that would definitely help.
Cheers
---Dave
