[PATCH v5 1/2] arm64/sve: Don't disable SVE on syscalls return

[Mark Brown]

On Wed, Nov 18, 2020 at 12:51:56PM +0000, Dave Martin wrote:
> On Tue, Nov 17, 2020 at 11:03:38PM +0000, Mark Brown wrote:
> > On Tue, Nov 17, 2020 at 06:16:20PM +0000, Dave Martin wrote:

> > > I think where we get in a tangle is that TIF_SVE conflates mechanism
> > > (whether the kernel tracks the full SVE state) with policy (whether
> > > userspace cares about the full SVE state).  At present, we can't
> > > describe the situation where we track SVE state but userspace doesn't
> > > care, so we either have to do nothing at all and just leave SVE on all
> > > the time, or we have to disable SVE at the first
> > > opportunity.

> > I think this is the current conflation that is causing issues but I
> > wouldn't state it quite like that, I don't know that *care* is quite the
> > right word for what userspace is doing here.  There's two pieces,
> > there's the SVE state in the registers and there's the ability to
> > execute SVE instructions without trapping.  When userspace executes a
> > SVE instruction we enable execution and also start tracking the register
> > state which we currently track in the single TIF_SVE flag.  When
> > userspace does a syscall the extra register state becomes undefined
> > (realistically we'll probably have to continue resetting it to zero as

> With regard to "don't care", I used this wording what userspace is doing
> is saying "I don't need what's in the SVE regs, feel free to throw it
> away if it helps."  Do you see anything wrong with this desciption?
> Maybe there's a subtlety here I've forgotten.  "Don't care" seemed a
> good fit for this concept, but I'm not attached to this form or words
> per se.

The issue I see is that it's not just the register state, it's also the
ability to execute without trapping - the ABI tells userspace it can't
use the register state any more, it's a bit of a jump to imply that a
syscall means that userspace is no longer interested in SVE.  "Don't
care" implies intent to no longer use it (at least for a while) which is
a bit stronger.

> > we currently do) and we currently don't the ability to track the ability
> > to execute the instructions separately to discarding that state so we
> > just disable both at once.  This means that the syscall overhead is
> > amplified for any task that mixes SVE and syscalls.

> Sure, though I think the difference between your model and mine is just
> semantics.  If enter userspace with SVE untrapped, then we have no
> choice but to track the full SVE regs from that point, so the trap
> control and state tracking are still not really independent concepts.

Right, none of this stuff is *exactly* independent of each other.

> If you prefer to think of TIF_SVE as purely a trap control and define
> another flag for "has full SVE state" then I don't have a problem with
> that; there is a choice of ways to slice up the multiple meanings
> TIF_SVE currently has.

> In these patches TIF_SVE_NEEDS_FLUSH means "SVE untrapped, but don't
> track full SVE state", which is arguably confusing when we have TIF_SVE
> clear (suggesting trapping).

The way I was thinking about it both SVE flags mean "we have SVE state"
and it's just a question of where.  As you say there's more than one way
to do it.

> > While we can't be in userspace in the middle column I'm not sure it's
> > entirely true to say that we can't usefully represent things here in
> > memory if we think about the execute part of things as well.  If we
> > schedule another task we can still track if we want to have SVE
> > instruction execution on return to userspace even if the only data we're
> > storing is the FPSIMD subset, avoiding a second SVE trap on the first
> > SVE instruction after a syscall while also storing less data in memory.

> Sure, we could have an additional state here.  I'm not convinced it's
> useful, or not expressible in another way, but if it keeps the middle
> column more orthogonal from TIF_FOREIGN_FPSTATE and so makes the code
> simpler to understand, then that could be a good thing.  We'd need to
> see what it looks like, I guess.

Looking at the places where we set it I think it'd at least be a lot
simpler to defer processing a transition until we interpret the flag at
which point we're not really gaining anything by saying the case with
invalidated SVE regs never gets represented.  I also very much like the
current property where TIF_FOREIGN_FPSTATE just gets set when we do
something with the state without needing to think what's there, it just
flags if we need to reload the state from memory or not which is very
helpful for reasoning about things both in terms of when we set the flag
and how we handle it.

> > > And finally, on sched-out (or any other fpsimd_save() event):
> > 
> > >         +---------------+               +---------------+
> > >         |               |               |               |
> > >         |  :            +---------------+            :  |
> > >         |  :            |               |            :  |
> > >         +--:------------+               +------------:--+
> > >         |  :         <=======  [*]  =======>         :  |
> > >         |  V            +---------------+            V  |
> > >         |               |               |               |
> > >         +---------------+               +---------------+
> > 
> > > ...at least in threory.

> > I agree up to this final schedule case since here we'd either be taking
> > a decision to force userspace to trap on the next SVE instruction or to
> > store and reload full SVE state in order to avoid that neither of which
> > seems ideal.  With the current patch if TIF_SVE_NEEDS_FLUSH is set we
> > only save the FPSIMD state and then restore it and switch to TIF_SVE
> > when scheduling the task again - this is masked a bit in the patch since
> > it does not update the existing code in fpsimd_save() as TIF_SVE is not
> > set when setting TIF_SVE_NEEDS_FLUSH so there's no change in that path.

> I think I missed this point.  It doesn't sound quite right to me: how
> will we ever turn SVE persistently off for a task?

As mentioned in the cover letter (which I inherited from Julien, it's
been this way since v1) we don't currently, the proposal mentioned in
the cover letter is to turn it off after some number of syscalls.

> I had thought that the purpose of this patch was to improve the low-
> overhead paths -- so with no context switch we can spin between the
> kernel and userspace without paying the code of extra traps of dumping/
> reloading the regs as we do now.  Once we enter a high-overhead path, we
> might as well be brutal.

That's not something that was called out by the cover letter and it's
not obvious to me that we want to actively seek to do that, though I can
see the argument for it.  I think I was approaching it from the point of
view that it seemed to simplify things to record the state at entry and
implement whatever new state we wanted on observation without having to
think about it in the meantime.

> If we save the full SVE regs, and defer the decision on disabling SVE
> until the next sched-in then that's probably reasonable: we could leave
> SVE enabled if the regs turn out not to need reloading (as when resuming
> the task after temporarily running some kernel thread -- we reach
> do_notify_resume() with TIF_FOREIGN_FPSTATE clear).  If so, the bottom-
> middle box does seem to describe the state of that task while scheduled
> out.

Yeah.

> My own view is that we should disable SVE on sched-out (or possibly at
> fpsimd_restore_current_state()) rather than doing anything more clever
> just yet -- because these options remain close to the current behaviour
> while still addressing the excessive trapping issue that prompted this
> change.

I think restore is likely to be a better candidate than sched-out if
we're going that way.

> > (the first column being either no SVE flags set or only SVE_REGS set.)
> > All entries should go to one of the !FFP cases.  Entry from SVE traps or
> > syscalls with SVE_EXEC set should go to SVE_EXEC, other entries with
> > SVE_EXEC already set should go to SVE_REGS+SVE_EXEC and all other
> > entries should go to !SVE/SVE_REGS.
> > 
> > Currently TIF_SVE maps onto SVE_REGS+SVE_EXEC and TIF_SVE_NO_FLUSH maps
> > onto SVE_EXEC.

> Agreed.  I don't see a big problem with this, and it removes the
> weirdness where we turn off the flag that appears to mean ("SVE
> enabled") when in the middle column.

> I still think it's a bit of a moot point, since we still have to resolve
> the middle states to a different state before we can safely enter
> userspace -- but that's not so different from the TIF_FOREIGN_FPSTATE
> case.

Yes, my feeling was that it lined up nicely with the TIF_FOREIGN_FPSTATE
handling since it all comes out as an overall "implement whatever we
ended up with" step and as a result minimizes the chances that we'll end
up doing anything expensive we didn't need to unintentionally down the
line - it seems more likely to end up being helpful going forwards.

> > > An alternative behaviour for syscalls would be:

> > >                                           TIF_SVE
> > >                !TIF_SVE    ,-------------------------.
> > > 	  ,--------------------------.
> > > 
> > >         +---------------+               +---------------+
> > >         |               |               |               |
> > >         |               +---------------+               |
> > >         |            ======>         <======            |
> > >         +---------------+               +---------------+
> > >         |               |               |               |
> > >         |               +---------------+               |
> > >         |               |               |               |
> > >         +---------------+               +---------------+

> > I worry that this might make it harder to follow in that you'd have to
> > understand why we're considering enabling SVE for tasks that never tried
> > to do anything with it.

> The decision on where to go from the middle box can be anything we like.
> In the actual implementation, the obvious thing to do is to always go
> back to the left-hand box if TIF_SVE is clear.

> The middle box just indicates the _potential_ for a decision.  But we
> don't always have to use that opportunity.

...

> Of couse, since this flexibility is probably not all that useful in
> practice, it doesn't necessarily have to be expressed in the code.  The
> picture is more of an abstraction.

Right, it was the idea of representing it in code more than imagining
that one could do it that was concerning me.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20201118/c176e6ac/attachment.sig>