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

[Alex Kogan <alex.kogan@oracle.com>]

> On Nov 20, 2019, at 10:16 AM, kbuild test robot <lkp@intel.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@intel.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