Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755119AbbEFGQK (ORCPT ); Wed, 6 May 2015 02:16:10 -0400 Received: from mail-wg0-f68.google.com ([74.125.82.68]:35958 "EHLO mail-wg0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754214AbbEFGQH (ORCPT ); Wed, 6 May 2015 02:16:07 -0400 From: Ingo Molnar X-Google-Original-From: Ingo Molnar Date: Wed, 6 May 2015 08:16:03 +0200 To: Dave Hansen Cc: linux-kernel@vger.kernel.org, Andy Lutomirski , Borislav Petkov , Fenghua Yu , "H. Peter Anvin" , Linus Torvalds , Oleg Nesterov , Thomas Gleixner Subject: Re: [PATCH 084/208] x86/fpu: Rename xsave.header::xstate_bv to 'xfeatures' Message-ID: <20150506061603.GA13720@gmail.com> References: <1430848300-27877-1-git-send-email-mingo@kernel.org> <1430848300-27877-6-git-send-email-mingo@kernel.org> <554904A6.8040503@linux.intel.com> <20150505181613.GA28562@gmail.com> <55490B1F.3080409@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55490B1F.3080409@linux.intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4549 Lines: 116 * Dave Hansen wrote: > On 05/05/2015 11:16 AM, Ingo Molnar wrote: > > We could put the SDM name into a comment, next to the field > > definition? Something like, if 'xfeatures' is too long: > > > > struct xstate_header { > > u64 xfeat; /* xstate components, SDM: XSTATE_BV */ > > u64 xfeat_comp; /* compacted xstate components, SDM: XCOMP_BV */ > > u64 reserved[6]; > > } __attribute__((packed)); > > When you're in the depths of the SDM and the kernel code, the fewer > context switches you have to make, the better. [...] But that's not the only consideration. While in general I'm all for following reference hardware documentation with names, there's a limit for how far we'll go in following stupid vendor names, and I think 'XSTATE_BV' and 'XCOMP_BV' are well beyond any sane limit (see further below my suggestion for better naming). > [...] I say this from the perspective of someone who's had a copy > of the SDM open to xsave* for about a month straight. If only one of us worked at the company that invented those nonsensical names and complex SDMs, and could complain to them? ;-) > In any case, having "xfeat" and "xfeat_comp" is a bad idea. They're > not really related concepts other than their bits refer to the same > states. They should not have such similar names. Agreed. > XSTATE_BV is the set of states written to the xsave area. > > XCOMP_BV is essentially always XCR0 (aka pcntxt_mask, aka > xfeatures_mask) or'd with bit 63. So how about this naming: /* * Mask of xstate components currently not in init state, * typically written to by XSAVE*. */ u64 xfeat_mask_used; /* SDM: XSTATE_BV */ /* * Mask of all state components saved/restored, plus the * compaction flag. (Note that the XRSTORS instruction caches * this value, and the next SAVES done for this same * area expects it to match, before it can perform the 'were * these registers modified' hardware optimization.) */ u64 xfeat_mask_all; /* SDM: XCOMP_BV */ (Note that I kept the SDM name easily greppable.) The 'compaction' aspect of 'xfeat_mask_all' is just an additional quirk that does not deserve to be represented in the primary naming: bit 63 of 'xfeat_mask_all' is set to 1 if the format is compacted: basically 'compaction' can be thought of as an additional, special 'xfeature' that modifies the offsets in the save area to eliminate holes. [*] Basically this naming tells us the biggest, most relevant differentiation between these two fields: - the 'xfeat_mask_used' field reflects the current, momentary, optimized state of the area. This mask is content dependent, and it is a subset of: - the 'xfeat_mask_all' field which reflects all states supported by that fpstate context. This mask is content independent. The compaction aspect of 'xfeat_mask_all' is obviously important to the hardware (and depending on its value the position of various registers in the save area are different), but secondary to the big picture. Note that once you have a good name, a lot of code becomes a lot more obvious - and I wish Intel did more than just googling for the first available historic QuickBASIC variable name when picking new CPU symbols. Thanks, Ingo [*] Btw., does Intel have any special plans with xstate compaction? AFAICS in Linux we just want to enable xfeat_mask_all to the max, including compaction, and never really modify it (in the task's lifetime). I'm also wondering whether there will be any real 'holes' in the xfeatures capability masks of future CPUs: right now xfeatures tend to be already 'compacted' (because new CPUs tend to support all xfeatures), so compaction mostly appears to be an academic feature. Or is there already hardware out there where it matter? Maybe once we get AVX512 in addition to MPX we can use compaction materially: as there will be lots of tasks without MPX state but with AVX512 state - in fact I suspect that will be the common case. OTOH MPX state is relatively small compared to AVX and AVX512 state, so skipping the hole won't buy us much, and the question is, how expensive is compaction, will save/restore be slower with compaction enabled? Has to be measured I suspect. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/