Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [RFC PATCH v1 1/2] x86/fpu: detect AVX task
To:     Tim Chen <tim.c.chen@linux.intel.com>,
        Aubrey Li <aubrey.li@intel.com>, tglx@linutronix.de,
        mingo@redhat.com, peterz@infradead.org, hpa@zytor.com
Cc:     ak@linux.intel.com, arjan@linux.intel.com,
        linux-kernel@vger.kernel.org
References: <1541528590-30296-1-git-send-email-aubrey.li@intel.com>
 <cd83910a-7197-9c49-3b93-22d699d887c4@linux.intel.com>
From:   "Li, Aubrey" <aubrey.li@linux.intel.com>
Message-ID: <216189f0-a411-8bba-99d2-c4661de0ff93@linux.intel.com>
Date:   Thu, 8 Nov 2018 08:31:53 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.1.1
MIME-Version: 1.0
In-Reply-To: <cd83910a-7197-9c49-3b93-22d699d887c4@linux.intel.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2018/11/8 1:41, Tim Chen wrote:
> On 11/06/2018 10:23 AM, Aubrey Li wrote:
> 
>> +static inline void update_avx_state(struct avx_state *avx)
>> +{
>> +	/*
>> +	 * Check if XGETBV with ECX = 1 supported. XGETBV with ECX = 1
>> +	 * returns the logical-AND of XCR0 and XINUSE. XINUSE is a bitmap
>> +	 * by which the processor tracks the status of various components.
>> +	 */
>> +	if (!use_xgetbv1()) {
>> +		avx->state = 0;
>> +		return;
>> +	}
>> +	/*
>> +	 * XINUSE is dynamic to track component state because VZEROUPPER
>> +	 * happens on every function end and reset the bitmap to the
>> +	 * initial configuration.
>> +	 *
>> +	 * State decay is introduced to solve the race condition between
>> +	 * context switch and a function end. State is aggressively set
>> +	 * once it's detected but need to be cleared by decay 3 context
>> +	 * switches
>> +	 */
>> +	if (xgetbv(XINUSE_STATE_BITMAP_INDEX) & XFEATURE_MASK_Hi16_ZMM) {
>> +		avx->state = 1;
>> +		avx->decay_count = AVX_STATE_DECAY_COUNT;
>> +	} else {
>> +		if (!avx->decay_count)
> 
> Seems like the check should be
> 
> 		if (avx->decay_count)
> 
> as we decrement the decay_count if it is non-zero.

Right, thanks to point this out, will fix in v2 soon.

Thanks,
-Aubrey
> 
>> +			avx->decay_count--;
>> +		else
>> +			avx->state = 0;
>> +	}
>> +}
> 
> Tim
>