Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <799d314f-17c6-4898-1363-4360996d89be@intel.com>
Date:   Thu, 28 Jul 2022 11:46:17 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH 1/2] x86/fpu: Measure the Latency of XSAVE and XRSTOR
Content-Language: en-US
To:     Yi Sun <yi.sun@intel.com>
CC:     <tony.luck@intel.com>, <heng.su@intel.com>,
        Dave Hansen <dave.hansen@intel.com>,
        <linux-kernel@vger.kernel.org>, <x86@kernel.org>
References: <20220723083800.824442-1-yi.sun@intel.com>
 <20220723083800.824442-2-yi.sun@intel.com>
From:   Sohil Mehta <sohil.mehta@intel.com>
In-Reply-To: <20220723083800.824442-2-yi.sun@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VUhCZ3M1WXNobUNaMFFlMjhhUWdQU0NLL0IwS050Y2NvUTdQYVQyWkR1dnZs?=
 =?utf-8?B?R3B2dEVaYzZ4R2pqdm5MbWVINVIvUVBQS05iNEZMcEZhVG5iN1AyOVNBZFVa?=
 =?utf-8?B?Z2RGYXJmTlJVOEVBUmNDdTkrcUFNNW5mVHVtUnNlNUtvVWt6TjJ1dXIvMjA3?=
 =?utf-8?B?dVB6WHdFQ0ZUekZmQWJWRWJrS0JOUWNWdE5Xb1orOGErcXpmdUhHL2tBOTVN?=
 =?utf-8?B?MWloU2tmelFBY3dKL0xyRFEzWW5NZVhhR1JYbVlGdHRKVlBCT1E2L3lqL044?=
 =?utf-8?B?VjYzM0U5VkFUOGtRbGlDSDBvUXhLMk41RXd3T0h6ZklDYVBGbHVJMHgyU2F5?=
 =?utf-8?B?MWdXbUQvQ0l5KzF1Y2pmLzNDYXhGbFdBa2hpb0hkMVROd3E5NThCWTVyVjZI?=
 =?utf-8?B?NlZMUzRmWHJZbXI2S2cvU3p0MFRQd2VleUtEL1poMjhPZGc3VHQ0SERFNncw?=
 =?utf-8?B?RkdJMWNkZ3RWRStmcHV3bTIzQ0lzcXNuTkk5eVIwQk1qTXUxaEdUZ3BVeFhp?=
 =?utf-8?B?N0IrNlIya2FWTlZsV3VCMmtSNEMrdnlpWnQ1UUxibHZxSUlFQi92YWs4TDU2?=
 =?utf-8?B?U3hXeWhJTzA5OTQ3bDkyL3dIUmZ6Vllyb2JHditkWUxpdnhYb0pQNFJrNmhs?=
 =?utf-8?B?eTJYeEJzczRBZlE2MVdoRFA1RmZyWFI3UFEwZVdTeDZ4UWpLRGdMYlhxSEY0?=
 =?utf-8?B?WU9LVVE3QXFwQ1NnZjl6KzJscnczY2pNUjNSQS9uUTgrNDg5S1J2M29mMURC?=
 =?utf-8?B?bFdmaTkxclFjSDkxSjFhUXI2UTdFK3RZbDBSdUhrWEZXVXRTMGl4eFdVcTEz?=
 =?utf-8?B?M1lkaFRZOXY2THB4TFpCS0tKSTNXaVhIdnJ0VkErbVo1d3Bwc0NyVVhkSmd2?=
 =?utf-8?B?Y2d3bGMvVURBTzZTTnI2NE1ib2dmRU01TW53U0taUjdLR2YxUGhTcWRNZ3NO?=
 =?utf-8?B?ZEIvYllSWVhsWVhwUUQ3VW1QQk1vNkRidkwyNXhaV3JFZFhSMzdKd3JsSlox?=
 =?utf-8?B?WVNyT0RBQ3J1K2lHKzlDQmd6dFdwU200aEdWZWpCdTNiZnRUM1ZVQ3hhSnZw?=
 =?utf-8?B?MEtwYWVTbmoxOEtCZnRHNys4SGxlWU1wUUVRK0tiU2hYUjFvSVpkU0FVVkV0?=
 =?utf-8?B?bHlMT2ZMekUyMWQ0UmNMVEw1UmFEU3ZveXhneDUwYy9RVldiQkJBQ1NXdDRn?=
 =?utf-8?B?N2VLclZITnBPZ1h3ekVyNjBmVEVzMWZZbk9aWjFVVTJzd1U3Uy9Fd1JNeE9S?=
 =?utf-8?B?eEwvWWtEMWN0MkRpL1Y2aGxxTVBrL05VaXY5Rmw0aEhIL09UWjlNYmZ5R1Jw?=
 =?utf-8?B?QTJ6U0FsMUNhOFVwUFY3cnZnVVVGdzZhV2RKeFhDVFdnVFRrTk9VcG8rdkVp?=
 =?utf-8?B?a25NMEtMRkpud3NjVmxVZ1BIV0xQWFRONDAyUGdFZVhWdGI1UnhMUFRyd3gx?=
 =?utf-8?B?ZXo2bWY1SjBhTmtCS2xFNVBwck9YQnlndWVlN3lhY3NzTHAxeEdaOHMyaUl2?=
 =?utf-8?B?SVZnNjM2c0VwWGkyb2ZPdGxXZVhMeWo5eTdvUmFtTHNqanpGTFBsYktQdCtv?=
 =?utf-8?B?akdJbFpuNERBQldxaERDQXo2UUxnREllaFlCeWFrMUZ1ZHBXRVFRZWtvZXhm?=
 =?utf-8?B?SnNpQ0c2QWpIUm95Unh5MmdvTGNPd1JSdC9MdnpMOWtMbVp6QTNUeTdkOTYx?=
 =?utf-8?B?NW1pSnYvbVJkZnFZem1ySU9zeWcwUTFIaGlNU0lITnlJSDVvRGZBQ2lzRlNr?=
 =?utf-8?B?SmROU1ZDRStDcnYwY0JkSy9hNzRnMi9WdnNkc3pPTmJpQzAxK1h4TW41ZzlX?=
 =?utf-8?B?TEI3YlJ0RThhZGZtb29VclQ4UVpuNmFSWVhxcTlpTkRWYTJOSVo4L0Q4SEU0?=
 =?utf-8?B?VjUybDBTZDNuOVY1ZUYrK0g2Um9qUmlFM1ROSlp6eHZkQ3ZWQjdRbEpmSENk?=
 =?utf-8?B?eDN0cWIrTjZNd25CVFovcDZmMnRhYXJiUUdldUJqV3RuV0h5aDdHNE0zajRm?=
 =?utf-8?B?Q2RHMk1NMWRHdkY2WjdIdlFyZXU2dXRFalBnWVVmMXNKVStvSllxUGhWNjdk?=
 =?utf-8?B?UEV4N3Y5TDVQbXZOVlptL0ZUdlY0ajRUM3llZjM5THZYajdOQUV2bkUxbENZ?=
 =?utf-8?Q?rtyKh1UHPMmfbs7ogGbOKSTYe?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 09ceca4c-781a-4648-c2bb-08da70c975c0
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3320.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Jul 2022 18:46:19.3900
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: iocrDnRMaslM5zpHdAVIaxbj8t9ndM6ouZGjuFD+Pq92LGAwnP4qowNZRzsT8pxGG8NMtJwYmAkpg/z5eJZy/g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR11MB3976
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_HI,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Some nits below:

On 7/23/2022 1:37 AM, Yi Sun wrote:
> Calculate the latency of instructions xsave and xrstor with new trace
> points x86_fpu_latency_xsave and x86_fpu_latency_xrstor.
> 
> The delta TSC can be calculated within a single trace event. 

s/can be/is

Another
> option considered was to have 2 separated trace events marking the
> start and finish of the xsave/xrstor instructions. The delta TSC was
> calculated from the 2 trace points in user space, but there was
> significant overhead added by the trace function itself.
> 

...

> 
> To enable it, CONFIG_X86_DEBUG_FPU and CONFIG_TRACEPOINTS are required.
> The compiler can get rid of all the extra crust when either of the two

s/can/will


> configs is disabled.
> 

> If both of the two configs are enabled, xsave/xrstor_tracing_enabled

s/two/

> would be reduced to a static check for tracing enabled. Thus, in the
> fast path there would be only 2 additional static checks.

...

> Leave the noise here instead of additional
> conditions before calling the x86_fpu_latency_* because that makes the
> latency more accurate and it's easy to filer the noise out by the

s/filer/filter

> following consuming script.

...

>   
> +DECLARE_EVENT_CLASS(x86_fpu_latency,
> +	TP_PROTO(struct fpstate *fpstate, u64 dtsc),

On x86, though TSC is the underlying counter for trace_clock(), it might 
be useful to keep this generic.

s/dtsc/latency

"latency" would match the printk below as well.

> +	TP_ARGS(fpstate, dtsc),
> +
> +	TP_STRUCT__entry(
> +		__field(struct fpstate *, fpstate)
> +		__field(u64, dtsc)
> +		__field(u64, rfbm)
> +		__field(u64, xinuse)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->fpstate = fpstate;
> +		__entry->dtsc = dtsc;
> +		__entry->rfbm = fpstate->xfeatures;
> +		__entry->xinuse = fpstate->regs.xsave.header.xfeatures;
> +	),
> +
> +	TP_printk("x86/fpu: latency:%lld RFBM:0x%llx XINUSE:0x%llx",
> +		__entry->dtsc,
> +		__entry->rfbm,
> +		__entry->xinuse
> +	)
> +);
> +

-Sohil