From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex =?utf-8?Q?Benn=C3=A9e?= Subject: Re: [PATCH v10 07/18] arm64: fpsimd: Eliminate task->mm checks Date: Thu, 24 May 2018 10:16:31 +0100 Message-ID: <87efi1tnv4.fsf@linaro.org> References: <1527005119-6842-1-git-send-email-Dave.Martin@arm.com> <1527005119-6842-8-git-send-email-Dave.Martin@arm.com> <20180523114812.GH55598@C02W217FHV2R.local> <20180523133158.GJ13470@e103592.cambridge.arm.com> <20180523145657.g7b6v2q2vxfqpoc4@armageddon.cambridge.arm.com> <20180523150337.GO13470@e103592.cambridge.arm.com> <20180524083350.GK55598@C02W217FHV2R.local> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-reply-to: <20180524083350.GK55598@C02W217FHV2R.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Christoffer Dall Cc: Christoffer Dall , Ard Biesheuvel , Marc Zyngier , Catalin Marinas , Will Deacon , kvmarm@lists.cs.columbia.edu, Dave Martin , linux-arm-kernel@lists.infradead.org List-Id: kvmarm@lists.cs.columbia.edu CkNocmlzdG9mZmVyIERhbGwgPGNocmlzdG9mZmVyLmRhbGxAYXJtLmNvbT4gd3JpdGVzOgoKPiBP biBXZWQsIE1heSAyMywgMjAxOCBhdCAwNDowMzozN1BNICswMTAwLCBEYXZlIE1hcnRpbiB3cm90 ZToKPj4gT24gV2VkLCBNYXkgMjMsIDIwMTggYXQgMDM6NTY6NTdQTSArMDEwMCwgQ2F0YWxpbiBN YXJpbmFzIHdyb3RlOgo+PiA+IE9uIFdlZCwgTWF5IDIzLCAyMDE4IGF0IDAyOjMxOjU5UE0gKzAx MDAsIERhdmUgUCBNYXJ0aW4gd3JvdGU6Cj4+ID4gPiBPbiBXZWQsIE1heSAyMywgMjAxOCBhdCAw MTo0ODoxMlBNICswMjAwLCBDaHJpc3RvZmZlciBEYWxsIHdyb3RlOgo+PiA+ID4gPiBPbiBUdWUs IE1heSAyMiwgMjAxOCBhdCAwNTowNTowOFBNICswMTAwLCBEYXZlIE1hcnRpbiB3cm90ZToKPj4g PiA+ID4gPiBUaGlzIGlzIHRydWUgYnkgY29uc3RydWN0aW9uIGhvd2V2ZXI6IFRJRl9GT1JFSUdO X0ZQU1RBVEUgaXMgbmV2ZXIKPj4gPiA+ID4gPiBjbGVhcmVkIGV4Y2VwdCB3aGVuIHJldHVybmlu ZyB0byB1c2Vyc3BhY2Ugb3IgcmV0dXJuaW5nIGZyb20gYQo+PiA+ID4gPiA+IHNpZ25hbDogdGh1 cywgZm9yIGEgdHJ1ZSBrZXJuZWwgdGhyZWFkIG5vIEZQU0lNRCBjb250ZXh0IGlzIGV2ZXIKPj4g PiA+ID4gPiBsb2FkZWQsIFRJRl9GT1JFSUdOX0ZQU1RBVEUgd2lsbCByZW1haW4gc2V0IGFuZCBu byBjb250ZXh0IHdpbGwKPj4gPiA+ID4gPiBldmVyIGJlIHNhdmVkLgo+PiA+ID4gPgo+PiA+ID4g PiBJIGRvbid0IHVuZGVyc3RhbmQgdGhpcyBjb25zdHJ1Y3Rpb24gcHJvb2Y7IGZyb20gbG9va2lu ZyBhdCB0aGUgcGF0Y2gKPj4gPiA+ID4gYmVsb3cgaXQgaXMgbm90IG9idmlvdXMgdG8gbWUgd2h5 IGZwc2ltZF90aHJlYWRfc3dpdGNoKCkgY2FuIG5ldmVyIGhhdmUKPj4gPiA+ID4gIXdyb25nX3Rh c2sgJiYgIXdyb25nX2NwdSBhbmQgdGhlcmVmb3JlIGNsZWFyIFRJRl9GT1JFSUdOX0ZQU1RBVEUg Zm9yIGEKPj4gPiA+ID4ga2VybmVsIHRocmVhZD8KPj4gPiA+Cj4+ID4gPiBMb29raW5nIGF0IHRo aXMgYWdhaW4sIEkgdGhpbmsgaXQgaXMgcG9vcmx5IHdvcmRlZC4gIFRoaXMgcGF0Y2ggYWltcyB0 bwo+PiA+ID4gbWFrZSBpdCB0cnVlIGJ5IGNvbnN0cnVjdGlvbiwgYnV0IGl0IGlzbid0IHByaW9y IHRvIHRoZSBwYXRjaC4KPj4gPiA+Cj4+ID4gPiBJJ20gdGVtcHRlZCB0byBkZWxldGUgdGhlIHBh cmFncmFwaDogdGhlIGFzc2VydGlvbiBvZiBib3RoIHVudHJ1ZSBhbmQKPj4gPiA+IG5vdCB0aGUg YmVzdCB3YXkgdG8ganVzdGlmeSB0aGF0IHRoaXMgcGF0Y2ggd29ya3MuCj4+ID4gPgo+PiA+ID4K Pj4gPiA+IEhvdyBhYm91dDoKPj4gPiA+Cj4+ID4gPiAtODwtCj4+ID4gPgo+PiA+ID4gVGhlIGNv bnRleHQgc3dpdGNoIGxvZ2ljIGFscmVhZHkgaXNvbGF0ZXMgdXNlciB0aHJlYWRzIGZyb20gZWFj aCBvdGhlci4KPj4gPiA+IFRoaXMsIGl0IGlzIHN1ZmZpY2llbnQgZm9yIGlzb2xhdGluZyB1c2Vy IHRocmVhZHMgZnJvbSB0aGUga2VybmVsLAo+Cj4gcy9UaGlzL1RodXMvID8KPgo+IEkgZG9uJ3Qg dW5kZXJzdGFuZCB3aGF0ICdpdCcgcmVmZXJzIHRvIGhlcmU/Cj4KPj4gPiA+IHNpbmNlIHRoZSBn b2FsIGVpdGhlciB3YXkgaXMgdG8gZW5zdXJlIHRoYXQgY29kZSBleGVjdXRpbmcgaW4gdXNlcnNw YWNlCj4+ID4gPiBjYW5ub3Qgc2VlIGFueSBGUFNJTUQgc3RhdGUgZXhjZXB0IGl0cyBvd24uICBU aHVzLCB0aGVyZSBpcyBubyBzcGVjaWFsCj4+ID4gPiBwcm9wZXJ0eSBvZiBrZXJuZWwgdGhyZWFk cyB0aGF0IHdlIGNhcmUgYWJvdXQgZXhjZXB0IHRoYXQgaXQgaXMKPj4gPiA+IHBvaW50bGVzcyB0 byBzYXZlIG9yIGxvYWQgRlBTSU1EIHJlZ2lzdGVyIHN0YXRlIGZvciB0aGVtLgo+Cj4gQWN0dWFs bHksIEknbSBub3QgcmVhbGx5IHN1cmUgd2hhdCB0aGlzIHBhcmFncmFwaCBpcyBnZXR0aW5nIGF0 Lgo+Cj4+ID4gPgo+PiA+ID4gQXQgd29yc3QsIHRoZSByZW1vdmFsIG9mIGFsbCB0aGUga2VybmVs IHRocmVhZCBzcGVjaWFsIGNhc2VzIGJ5IHRoaXMKPj4gPiA+IHBhdGNoIHdvdWxkIHRodXMgc3B1 cmlvdXNseSBsb2FkIGFuZCBzYXZlIHN0YXRlIGZvciBrZXJuZWwgdGhyZWFkcyB3aGVuCj4+ID4g PiB1bm5lY2Vzc2FyeS4KPj4gPiA+Cj4+ID4gPiBCdXQgdGhlIGNvbnRleHQgc3dpdGNoIGxvZ2lj IGlzIGFscmVhZHkgZGVsaWJlcmF0ZWx5IG9wdGltaXNlZCB0byBkZWZlcgo+PiA+ID4gcmVsb2Fk cyBvZiB0aGUgcmVncyB1bnRpbCByZXRfdG9fdXNlciAob3Igc2lncmV0dXJuIGFzIGEgc3BlY2lh bCBjYXNlKSwKPj4gPiA+IHdoaWNoIGtlcm5lbCB0aHJlYWRzIGJ5IGRlZmluaXRpb24gbmV2ZXIg cmVhY2guCj4+ID4gPgo+PiA+ID4gLT44LQo+PiA+Cj4+ID4gVGhlICJhdCB3b3JzdCIgcGFyYWdy YXBoIG1ha2VzIGl0IGxvb2sgbGlrZSBpdCBjb3VsZCBoYXBwZW4gKGF0IGxlYXN0Cj4+ID4gdW50 aWwgeW91IHJlYWNoIHRoZSBsYXN0IHBhcmFncmFwaCkuIE1heWJlIHlvdSBjYW4ganVzdCBzYXkg dGhhdAo+PiA+IHdyb25nX3Rhc2sgYW5kIHdyb25nX2NwdSAod2l0aCB0aGUgZnBzaW1kX2NwdSA9 IE5SX0NQVVMgYWRkaXRpb24pIGFyZQo+PiA+IGFsd2F5cyB0cnVlIGZvciBrZXJuZWwgdGhyZWFk cy4gWW91IHNob3VsZCBwcm9iYWJseSBtZW50aW9uIHRoaXMgaW4gYQo+PiA+IGNvbW1lbnQgaW4g dGhlIGNvZGUgYXMgd2VsbC4KPj4KPj4gV2hhdCBpZiBJIGp1c3QgZGVsZXRlIHRoZSBzZWNvbmQg cGFyYWdyYXBoLCBhbmQgcmVtb3ZlIHRoZSAiQnV0IiBmcm9tCj4+IHRoZSBzdGFydCBvZiB0aGUg dGhpcmQsIGFuZCBhcHBlbmQ6Cj4+Cj4+ICJBcyBhIHJlc3VsdCwgdGhlIHdyb25nX3Rhc2sgYW5k IHdyb25nX2NwdSB0ZXN0cyBpbgo+PiBmcHNpbWRfdGhyZWFkX3N3aXRjaCgpIHdpbGwgYWx3YXlz IHlpZWxkIGZhbHNlIGZvciBrZXJuZWwgdGhyZWFkcy4iCj4+Cj4+IC4uLndpdGggYSBzaW1pbGFy IGNvbW1lbnQgaW4gdGhlIGNvZGU/Cj4KPiAuLi53aXRoIGEgcmlzayBvZiBiZWluZyBhIGJpdCBv dmVyLXBlZGFudGljIGFuZCBhbm5veWluZywgbWF5IEkgc3VnZ2VzdAo+IHRoZSBmb2xsb3dpbmcg Y29tcGxldGUgY29tbWl0IHRleHQ6Cj4KPiAtLS0tLS04PC0tLS0tLQo+IEN1cnJlbnRseSB0aGUg RlBTSU1EIGhhbmRsaW5nIGNvZGUgdXNlcyB0aGUgY29uZGl0aW9uIHRhc2stPm1tID09Cj4gTlVM TCBhcyBhIGhpbnQgdGhhdCB0YXNrIGhhcyBubyBGUFNJTUQgcmVnaXN0ZXIgY29udGV4dC4KPgo+ IFRoZSAtPm1tIGNoZWNrIGlzIG9ubHkgdGhlcmUgdG8gZmlsdGVyIG91dCB0YXNrcyB0aGF0IGNh bm5vdAo+IHBvc3NpYmx5IGhhdmUgRlBTSU1EIGNvbnRleHQgbG9hZGVkLCBmb3Igb3B0aW1pc2F0 aW9uIHB1cnBvc2VzLgo+IEhvd2V2ZXIsIFRJRl9GT1JFSUdOX0ZQU1RBVEUgbXVzdCBhbHdheXMg YmUgY2hlY2tlZCBhbnl3YXkgYmVmb3JlCj4gc2F2aW5nIEZQU0lNRCBjb250ZXh0IGJhY2sgdG8g bWVtb3J5LiAgRm9yIHRoaXMgcmVhc29uLCB0aGUgLT5tbQo+IGNoZWNrcyBhcmUgbm90IHVzZWZ1 bCwgcHJvdmlkaW5nIHRoYXQgdGhhdCBUSUZfRk9SRUlHTl9GUFNUQVRFIGlzCj4gbWFpbnRhaW5l ZCBwcm9wZXJseSBmb3Iga2VybmVsIHRocmVhZHMuCj4KPiBGUFNJTUQgY29udGV4dCBpcyBuZXZl ciBwcmVzZXJ2ZWQgZm9yIGtlcm5lbCB0aHJlYWRzIGFjcm9zcyBhIGNvbnRleHQKPiBzd2l0Y2gg YW5kIHRoZXJlZm9yZSBUSUZfRk9SRUlHTl9GUFNUQVRFIHNob3VsZCBhbHdheXMgYmUgdHJ1ZSBm b3IKPiBrZXJuZWwgdGhyZWFkcy4gIFRoaXMgaXMgaW5kZWVkIHRoZSBjYXNlLCBhcyB0aGUgd3Jv bmdfdGFzayBhbmQKPiB3cm9uZ19jcHUgdGVzdHMgaW4gZnBzaW1kX3RocmVhZF9zd2l0Y2goKSB3 aWxsIGFsd2F5cyB5aWVsZCBmYWxzZSBmb3IKPiBrZXJuZWwgdGhyZWFkcy4KPgo+IEZ1cnRoZXIs IHRoZSBjb250ZXh0IHN3aXRjaCBsb2dpYyBpcyBhbHJlYWR5IGRlbGliZXJhdGVseSBvcHRpbWlz ZWQgdG8KPiBkZWZlciByZWxvYWRzIG9mIHRoZSBGUFNJTUQgY29udGV4dCB1bnRpbCByZXRfdG9f dXNlciAob3Igc2lncmV0dXJuIGFzIGEKPiBzcGVjaWFsIGNhc2UpLCB3aGljaCBrZXJuZWwgdGhy ZWFkcyBieSBkZWZpbml0aW9uIG5ldmVyIHJlYWNoLCBhbmQKPiB0aGVyZWZvcmUgdGhpcyBjaGFu Z2UgaW50cm9kdWNlcyBubyBhZGRpdGlvbmFsIHdvcmsgaW4gdGhlIGNyaXRpY2FsCj4gcGF0aC4K Pgo+IFRoaXMgcGF0Y2ggcmVtb3ZlcyB0aGUgcmVkdW5kYW50IGNoZWNrcyBhbmQgc3BlY2lhbC1j YXNlIGNvZGUuCj4gLS0tLS0tODwtLS0tLS0KCkZXSVcgSSBwcmVmZXIgdGhpcyB2ZXJzaW9uIGZv ciB0aGUgY29tbWl0IHRleHQuCgotLQpBbGV4IEJlbm7DqWUKCl9fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fCmxpbnV4LWFybS1rZXJuZWwgbWFpbGluZyBsaXN0 CmxpbnV4LWFybS1rZXJuZWxAbGlzdHMuaW5mcmFkZWFkLm9yZwpodHRwOi8vbGlzdHMuaW5mcmFk ZWFkLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2xpbnV4LWFybS1rZXJuZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 From: alex.bennee@linaro.org (Alex =?utf-8?Q?Benn=C3=A9e?=) Date: Thu, 24 May 2018 10:16:31 +0100 Subject: [PATCH v10 07/18] arm64: fpsimd: Eliminate task->mm checks In-Reply-To: <20180524083350.GK55598@C02W217FHV2R.local> References: <1527005119-6842-1-git-send-email-Dave.Martin@arm.com> <1527005119-6842-8-git-send-email-Dave.Martin@arm.com> <20180523114812.GH55598@C02W217FHV2R.local> <20180523133158.GJ13470@e103592.cambridge.arm.com> <20180523145657.g7b6v2q2vxfqpoc4@armageddon.cambridge.arm.com> <20180523150337.GO13470@e103592.cambridge.arm.com> <20180524083350.GK55598@C02W217FHV2R.local> Message-ID: <87efi1tnv4.fsf@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Christoffer Dall writes: > On Wed, May 23, 2018 at 04:03:37PM +0100, Dave Martin wrote: >> On Wed, May 23, 2018 at 03:56:57PM +0100, Catalin Marinas wrote: >> > On Wed, May 23, 2018 at 02:31:59PM +0100, Dave P Martin wrote: >> > > On Wed, May 23, 2018 at 01:48:12PM +0200, Christoffer Dall wrote: >> > > > On Tue, May 22, 2018 at 05:05:08PM +0100, Dave Martin wrote: >> > > > > This is true by construction however: TIF_FOREIGN_FPSTATE is never >> > > > > cleared except when returning to userspace or returning from a >> > > > > signal: thus, for a true kernel thread no FPSIMD context is ever >> > > > > loaded, TIF_FOREIGN_FPSTATE will remain set and no context will >> > > > > ever be saved. >> > > > >> > > > I don't understand this construction proof; from looking at the patch >> > > > below it is not obvious to me why fpsimd_thread_switch() can never have >> > > > !wrong_task && !wrong_cpu and therefore clear TIF_FOREIGN_FPSTATE for a >> > > > kernel thread? >> > > >> > > Looking at this again, I think it is poorly worded. This patch aims to >> > > make it true by construction, but it isn't prior to the patch. >> > > >> > > I'm tempted to delete the paragraph: the assertion of both untrue and >> > > not the best way to justify that this patch works. >> > > >> > > >> > > How about: >> > > >> > > -8<- >> > > >> > > The context switch logic already isolates user threads from each other. >> > > This, it is sufficient for isolating user threads from the kernel, > > s/This/Thus/ ? > > I don't understand what 'it' refers to here? > >> > > since the goal either way is to ensure that code executing in userspace >> > > cannot see any FPSIMD state except its own. Thus, there is no special >> > > property of kernel threads that we care about except that it is >> > > pointless to save or load FPSIMD register state for them. > > Actually, I'm not really sure what this paragraph is getting at. > >> > > >> > > At worst, the removal of all the kernel thread special cases by this >> > > patch would thus spuriously load and save state for kernel threads when >> > > unnecessary. >> > > >> > > But the context switch logic is already deliberately optimised to defer >> > > reloads of the regs until ret_to_user (or sigreturn as a special case), >> > > which kernel threads by definition never reach. >> > > >> > > ->8- >> > >> > The "at worst" paragraph makes it look like it could happen (at least >> > until you reach the last paragraph). Maybe you can just say that >> > wrong_task and wrong_cpu (with the fpsimd_cpu = NR_CPUS addition) are >> > always true for kernel threads. You should probably mention this in a >> > comment in the code as well. >> >> What if I just delete the second paragraph, and remove the "But" from >> the start of the third, and append: >> >> "As a result, the wrong_task and wrong_cpu tests in >> fpsimd_thread_switch() will always yield false for kernel threads." >> >> ...with a similar comment in the code? > > ...with a risk of being a bit over-pedantic and annoying, may I suggest > the following complete commit text: > > ------8<------ > Currently the FPSIMD handling code uses the condition task->mm == > NULL as a hint that task has no FPSIMD register context. > > The ->mm check is only there to filter out tasks that cannot > possibly have FPSIMD context loaded, for optimisation purposes. > However, TIF_FOREIGN_FPSTATE must always be checked anyway before > saving FPSIMD context back to memory. For this reason, the ->mm > checks are not useful, providing that that TIF_FOREIGN_FPSTATE is > maintained properly for kernel threads. > > FPSIMD context is never preserved for kernel threads across a context > switch and therefore TIF_FOREIGN_FPSTATE should always be true for > kernel threads. This is indeed the case, as the wrong_task and > wrong_cpu tests in fpsimd_thread_switch() will always yield false for > kernel threads. > > Further, the context switch logic is already deliberately optimised to > defer reloads of the FPSIMD context until ret_to_user (or sigreturn as a > special case), which kernel threads by definition never reach, and > therefore this change introduces no additional work in the critical > path. > > This patch removes the redundant checks and special-case code. > ------8<------ FWIW I prefer this version for the commit text. -- Alex Benn?e