Re: [PATCH] crypto: x86/crc32c-intel - Don't match some Zhaoxin CPUs
On December 22, 2020 3:27:33 AM GMT+08:00, h...@zytor.com wrote: >On December 20, 2020 6:46:25 PM PST, tonywwang...@zhaoxin.com wrote: >>On December 16, 2020 1:56:45 AM GMT+08:00, Eric Biggers >> wrote: >>>On Tue, Dec 15, 2020 at 10:15:29AM +0800, Tony W Wang-oc wrote: On 15/12/2020 04:41, Eric Biggers wrote: > On Mon, Dec 14, 2020 at 10:28:19AM +0800, Tony W Wang-oc wrote: >> On 12/12/2020 01:43, Eric Biggers wrote: >>> On Fri, Dec 11, 2020 at 07:29:04PM +0800, Tony W Wang-oc wrote: The driver crc32c-intel match CPUs supporting >>>X86_FEATURE_XMM4_2. On platforms with Zhaoxin CPUs supporting this X86 feature, >>When crc32c-intel and crc32c-generic are both registered, system >>will use crc32c-intel because its .cra_priority is greater than crc32c-generic. This case expect to use crc32c-generic driver >>>for some Zhaoxin CPUs to get performance gain, So remove these >>>Zhaoxin CPUs support from crc32c-intel. Signed-off-by: Tony W Wang-oc >>> >>> Does this mean that the performance of the crc32c instruction >on >>>those CPUs is >>> actually slower than a regular C implementation? That's very >>>weird. >>> >> >> From the lmbench3 Create and Delete file test on those chips, I >>>think yes. >> > > Did you try measuring the performance of the hashing itself, and >>>not some > higher-level filesystem operations? > Yes. Was testing on these Zhaoxin CPUs, the result is that with the >>>same input value the generic C implementation takes fewer time than the crc32c instruction implementation. >>> >>>And that is really "working as intended"? >> >>These CPU's crc32c instruction is not working as intended. >> >> Why do these CPUs even >>>declare that >>>they support the crc32c instruction, when it is so slow? >>> >> >>The presence of crc32c and some other instructions supports are >>enumerated by CPUID.01:ECX[SSE4.2] = 1, other instructions are ok >>except the crc32c instruction. >> >>>Are there any other instruction sets (AES-NI, PCLMUL, SSE, SSE2, AVX, >>>etc.) that >>>these CPUs similarly declare support for but they are uselessly slow? >> >>No. >> >>Sincerely >>Tonyw >> >>> >>>- Eric > >Then the right thing to do is to disable the CPUID bit in the >vendor-specific startup code. This way makes these CPUs do not support all instruction sets enumerated by CPUID.01:ECX[SSE4.2]. While only crc32c instruction is slow, just expect the crc32c-intel driver do not match these CPUs. Sincerely Tonyw
Re: [PATCH] crypto: x86/crc32c-intel - Don't match some Zhaoxin CPUs
On December 16, 2020 1:56:45 AM GMT+08:00, Eric Biggers wrote: >On Tue, Dec 15, 2020 at 10:15:29AM +0800, Tony W Wang-oc wrote: >> >> On 15/12/2020 04:41, Eric Biggers wrote: >> > On Mon, Dec 14, 2020 at 10:28:19AM +0800, Tony W Wang-oc wrote: >> >> On 12/12/2020 01:43, Eric Biggers wrote: >> >>> On Fri, Dec 11, 2020 at 07:29:04PM +0800, Tony W Wang-oc wrote: >> The driver crc32c-intel match CPUs supporting >X86_FEATURE_XMM4_2. >> On platforms with Zhaoxin CPUs supporting this X86 feature, When >> crc32c-intel and crc32c-generic are both registered, system will >> use crc32c-intel because its .cra_priority is greater than >> crc32c-generic. This case expect to use crc32c-generic driver >for >> some Zhaoxin CPUs to get performance gain, So remove these >Zhaoxin >> CPUs support from crc32c-intel. >> >> Signed-off-by: Tony W Wang-oc >> >>> >> >>> Does this mean that the performance of the crc32c instruction on >those CPUs is >> >>> actually slower than a regular C implementation? That's very >weird. >> >>> >> >> >> >> From the lmbench3 Create and Delete file test on those chips, I >think yes. >> >> >> > >> > Did you try measuring the performance of the hashing itself, and >not some >> > higher-level filesystem operations? >> > >> >> Yes. Was testing on these Zhaoxin CPUs, the result is that with the >same >> input value the generic C implementation takes fewer time than the >> crc32c instruction implementation. >> > >And that is really "working as intended"? These CPU's crc32c instruction is not working as intended. Why do these CPUs even >declare that >they support the crc32c instruction, when it is so slow? > The presence of crc32c and some other instructions supports are enumerated by CPUID.01:ECX[SSE4.2] = 1, other instructions are ok except the crc32c instruction. >Are there any other instruction sets (AES-NI, PCLMUL, SSE, SSE2, AVX, >etc.) that >these CPUs similarly declare support for but they are uselessly slow? No. Sincerely Tonyw > >- Eric
答复: [PATCH v1 1/3] x86/cpu: Create Zhaoxin processors architecture support file
On Thu, May 23, 2019, Thomas Gleixner wrote: >> + * This file is licensed under the terms of the GNU General >> + * License v2.0 or later. See file COPYING for details. > >Please remove this boilerplate text. The SPDX license identifier is >sufficient, but looking at that: > >> +// SPDX-License-Identifier: GPL-2.0 > >That clearly disagrees with your boilerplate text which says 'v2.0 or >later'. Assumed that you want or later, then the SPDX id needs to say so. > >// SPDX-License-Identifier: GPL-2.0-or-later Ok, I will adjust this text after sort this out with our lawyers. Thanks TonyWWang-oc
答复: [PATCH v1 1/3] x86/cpu: Create Zhaoxin processors architecture support file
On Thu, May 23, 2019, Borislav Petkov wrote: >> This patch is totally corrupted, with leading spaces dropped and tabs >> turned into spaces. Please read the email client documentation in the >> kernel directory for how to fix your email client, or just use 'git >> send-email' to send the patches out directly. > >.. and before you do that, run them all through checkpatch.pl and apply >common sense when fixing the warnings from it: Ok, I will run my patches through checkpatch.pl and modify them properly before resend. Thanks TonyWWang-oc
答复: [PATCH v1 1/3] x86/cpu: Create Zhaoxin processors architecture support file
On Thu, May 23, 2019, gre...@linuxfoundation.org wrote:
>> To identify Zhaoxin CPU, add a new vendor type X86_VENDOR_ZHAOXIN
>> for system recognition.
>>
>> Signed-off-by: TonyWWang
>
>Some basic patch tips. Your From: line needs to match the signed-off-by
>line, which is not true here. Also, please use your name with ' '
>characters.
Thank you, I will follow these tips.
>
>> +static void init_zhaoxin_cap(struct cpuinfo_x86 *c)
>> +{
>> + u32 lo, hi;
>> +
>> + /* Test for Extended Feature Flags presence */
>> + if (cpuid_eax(0xC000) >= 0xC001) {
>> + u32 tmp = cpuid_edx(0xC001);
>
>This patch is totally corrupted, with leading spaces dropped and tabs
>turned into spaces. Please read the email client documentation in the
>kernel directory for how to fix your email client, or just use 'git
>send-email' to send the patches out directly.
Sorry for that! I was made a mistake to send the patch with HTML format.
I will set up my email client correctly.
Thanks
TonyWWang-oc
答复: [PATCH v1 1/3] x86/cpu: Create Zhaoxin processors architecture support file
On Thu, May 23, 2019, gre...@linuxfoundation.org wrote: >> + * This file is licensed under the terms of the GNU General >> + * License v2.0 or later. See file COPYING for details. > >That contridicts the first line of the file :( > >Please sort this out with your lawyers and resend it corrected. > >Also, gpl "boilerplate" text like this does not need to be in the file >if you just include the spdx line. Ok, I will sort this out with our lawyers, then adjust this text correctly before resend it. Thanks TonyWWang-oc
