linux-kernel - Re: [PATCH v6 3/5] locking/qspinlock: Introduce CNA into the slow path of qspinlock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Fri, 22 Nov 2019 14:52:23 -0500
From:   Alex Kogan <alex.kogan@...cle.com>
To:     Waiman Long <longman@...hat.com>
Cc:     kbuild test robot <lkp@...el.com>, kbuild-all@...ts.01.org,
        linux@...linux.org.uk, peterz@...radead.org, mingo@...hat.com,
        will.deacon@....com, arnd@...db.de, linux-arch@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        tglx@...utronix.de, bp@...en8.de, hpa@...or.com, x86@...nel.org,
        guohanjun@...wei.com, jglauber@...vell.com,
        steven.sistare@...cle.com, daniel.m.jordan@...cle.com,
        dave.dice@...cle.com, rahul.x.yadav@...cle.com
Subject: Re: [PATCH v6 3/5] locking/qspinlock: Introduce CNA into the slow
 path of qspinlock



> On Nov 22, 2019, at 2:29 PM, Waiman Long <longman@...hat.com> wrote:
> 
> On 11/22/19 1:28 PM, Alex Kogan wrote:
>> 
>>> On Nov 20, 2019, at 10:16 AM, kbuild test robot <lkp@...el.com> wrote:
>>> 
>>> Hi Alex,
>>> 
>>> Thank you for the patch! Yet something to improve:
>>> 
>>> [auto build test ERROR on linus/master]
>>> [also build test ERROR on v5.4-rc8 next-20191120]
>>> [if your patch is applied to the wrong git tree, please drop us a note to help
>>> improve the system. BTW, we also suggest to use '--base' option to specify the
>>> base tree in git format-patch, please see https://urldefense.proofpoint.com/v2/url?u=https-3A__stackoverflow.com_a_37406982&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Hvhk3F4omdCk-GE1PTOm3Kn0A7ApWOZ2aZLTuVxFK4k&m=BxEt1232ccGlMGDinAB0QAUaTFyl-m5sp4C-crHjpoU&s=OzzQqg4fTDV55X-y4vbnGeXoJaPHSvO_EfrUQnMVRHc&e= ]
>>> 
>>> url:    https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_0day-2Dci_linux_commits_Alex-2DKogan_locking-2Dqspinlock-2DRename-2Dmcs-2Dlock-2Dunlock-2Dmacros-2Dand-2Dmake-2Dthem-2Dmore-2Dgeneric_20191109-2D180535&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Hvhk3F4omdCk-GE1PTOm3Kn0A7ApWOZ2aZLTuVxFK4k&m=BxEt1232ccGlMGDinAB0QAUaTFyl-m5sp4C-crHjpoU&s=uE7ZeYXOFiu09PUVjnCntEe2rR5x_QxS6dEW9twpfok&e= 
>>> base:   https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Hvhk3F4omdCk-GE1PTOm3Kn0A7ApWOZ2aZLTuVxFK4k&m=BxEt1232ccGlMGDinAB0QAUaTFyl-m5sp4C-crHjpoU&s=aAKxuXc_c7OF0ffioQfVsIB6H-4Sd9PYxSM7kurm2ig&e=  0058b0a506e40d9a2c62015fe92eb64a44d78cd9
>>> config: i386-randconfig-f003-20191120 (attached as .config)
>>> compiler: gcc-7 (Debian 7.4.0-14) 7.4.0
>>> reproduce:
>>>       # save the attached .config to linux build tree
>>>       make ARCH=i386 
>>> 
>>> If you fix the issue, kindly add following tag
>>> Reported-by: kbuild test robot <lkp@...el.com>
>>> 
>>> All error/warnings (new ones prefixed by >>):
>>> 
>>>  In file included from include/linux/export.h:42:0,
>>>                   from include/linux/linkage.h:7,
>>>                   from include/linux/kernel.h:8,
>>>                   from include/linux/list.h:9,
>>>                   from include/linux/smp.h:12,
>>>                   from kernel/locking/qspinlock.c:16:
>>>  kernel/locking/qspinlock_cna.h: In function 'cna_init_nodes':
>>>>> include/linux/compiler.h:350:38: error: call to '__compiletime_assert_80' declared with attribute error: BUILD_BUG_ON failed: sizeof(struct cna_node) > sizeof(struct qnode)
>>>    _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
>>>                                        ^
>>>  include/linux/compiler.h:331:4: note: in definition of macro '__compiletime_assert'
>>>      prefix ## suffix();    \
>>>      ^~~~~~
>>>  include/linux/compiler.h:350:2: note: in expansion of macro '_compiletime_assert'
>>>    _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
>>>    ^~~~~~~~~~~~~~~~~~~
>>>  include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
>>>   #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
>>>                                       ^~~~~~~~~~~~~~~~~~
>>>  include/linux/build_bug.h:50:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
>>>    BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
>>>    ^~~~~~~~~~~~~~~~
>>>>> kernel/locking/qspinlock_cna.h:80:2: note: in expansion of macro 'BUILD_BUG_ON'
>>>    BUILD_BUG_ON(sizeof(struct cna_node) > sizeof(struct qnode));
>>>    ^~~~~~~~~~~~
>> Consider the following definition of qnode:
>> 
>> struct qnode {
>> 	struct mcs_spinlock mcs;
>> #if defined(CONFIG_PARAVIRT_SPINLOCKS) || defined(CONFIG_NUMA_AWARE_SPINLOCKS)
>> 	long reserved[2];
>> #endif
>> };
>> 
>> and this is how cna_node is defined:
>> 
>> struct cna_node {
>> 	struct mcs_spinlock	mcs;
>> 	int			numa_node;
>> 	u32			encoded_tail;
>> 	u32			pre_scan_result; /* 0, 1, 2 or encoded tail */
>> 	u32			intra_count;
>> };
>> 
>> Since long is 32 bit on i386, we get the compilation error above.
>> 
>> We can try and squeeze CNA-specific fields into 64 bit on i386 (or any 32bit 
>> architecture for that matter). Note that an encoded tail pointer requires up 
>> to 24 bits, and we have two of those. We would want different field encodings 
>> for 32 vs 64bit architectures, and this all will be quite ugly.
>> 
>> So instead we should probably either change the definition of @reserved in qnode 
>> to long long, or perhaps disable CNA on 32bit architectures altogether?
>> I would certainly prefer the former, especially as it requires the least amount 
>> of code/config changes.
>> 
>> Any objections / thoughts?
>> 
>> Thanks,
>> — Alex
>> 
> The easy way out is to restrict NUMA qspinlock to 64-bit only. There
> aren't that many 32-bit NUMA systems out there that we have to worry about.
> 
> Just add "depends on 64BIT" to the config entry.
Ok, will do.

Thanks,
— Alex