| 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
| ||
|
Message-ID: <IA1PR11MB6098730609C110664EED7904F4F12@IA1PR11MB6098.namprd11.prod.outlook.com>
Date: Fri, 7 Feb 2025 06:11:58 +0000
From: "Xu, Even" <even.xu@...el.com>
To: David Laight <david.laight.linux@...il.com>
CC: lkp <lkp@...el.com>, "oe-kbuild-all@...ts.linux.dev"
<oe-kbuild-all@...ts.linux.dev>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, Jiri Kosina <jikos@...nel.org>, "Sun,
Xinpeng" <xinpeng.sun@...el.com>, Srinivas Pandruvada
<srinivas.pandruvada@...ux.intel.com>, Mark Pearson
<mpearson-lenovo@...ebb.ca>
Subject: RE: drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38:
sparse: sparse: dubious: !x | !y
> -----Original Message-----
> From: David Laight <david.laight.linux@...il.com>
> Sent: Thursday, February 6, 2025 5:05 PM
> To: Xu, Even <even.xu@...el.com>
> Cc: lkp <lkp@...el.com>; oe-kbuild-all@...ts.linux.dev; linux-
> kernel@...r.kernel.org; Jiri Kosina <jikos@...nel.org>; Sun, Xinpeng
> <xinpeng.sun@...el.com>; Srinivas Pandruvada
> <srinivas.pandruvada@...ux.intel.com>; Mark Pearson <mpearson-
> lenovo@...ebb.ca>
> Subject: Re: drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse:
> sparse: dubious: !x | !y
>
> On Thu, 6 Feb 2025 02:50:17 +0000
> "Xu, Even" <even.xu@...el.com> wrote:
>
> > > -----Original Message-----
> > > From: David Laight <david.laight.linux@...il.com>
> > > Sent: Thursday, February 6, 2025 5:57 AM
> > > To: Xu, Even <even.xu@...el.com>
> > > Cc: lkp <lkp@...el.com>; oe-kbuild-all@...ts.linux.dev; linux-
> > > kernel@...r.kernel.org; Jiri Kosina <jikos@...nel.org>; Sun, Xinpeng
> > > <xinpeng.sun@...el.com>; Srinivas Pandruvada
> > > <srinivas.pandruvada@...ux.intel.com>; Mark Pearson <mpearson-
> > > lenovo@...ebb.ca>
> > > Subject: Re: drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38:
> sparse:
> > > sparse: dubious: !x | !y
> > >
> > > On Wed, 5 Feb 2025 02:56:05 +0000
> > > "Xu, Even" <even.xu@...el.com> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: David Laight <david.laight.linux@...il.com>
> > > > > Sent: Thursday, January 30, 2025 6:22 AM
> > > > > To: lkp <lkp@...el.com>
> > > > > Cc: Xu, Even <even.xu@...el.com>; oe-kbuild-all@...ts.linux.dev;
> > > > > linux- kernel@...r.kernel.org; Jiri Kosina <jikos@...nel.org>;
> > > > > Sun, Xinpeng <xinpeng.sun@...el.com>; Srinivas Pandruvada
> > > > > <srinivas.pandruvada@...ux.intel.com>; Mark Pearson <mpearson-
> > > > > lenovo@...ebb.ca>
> > > > > Subject: Re: drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38:
> > > sparse:
> > > > > sparse: dubious: !x | !y
> > > > >
> > > > > On Wed, 29 Jan 2025 22:02:59 +0800 kernel test robot
> > > > > <lkp@...el.com> wrote:
> > > > >
> > > > > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> > > master
> > > > > > head: 05dbaf8dd8bf537d4b4eb3115ab42a5fb40ff1f5
> > > > > > commit: a688404b2e20f00cce6d0a2b888ef4ca9154e144 HID: intel-thc-
> hid:
> > > > > intel-thc: Add THC DMA interfaces
> > > > > > date: 3 weeks ago
> > > > > > config: x86_64-allyesconfig
> > > > > > (https://download.01.org/0day-ci/archive/20250129/202501292144
> > > > > > .eFD
> > > > > > q4ov
> > > > > > r-lkp@...el.com/config)
> > > > > > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is
> > > > > > a
> > > > > > W=1
> > > > > > build):
> > > > > > (https://download.01.org/0day-ci/archive/20250129/202501292144
> > > > > > .eFD
> > > > > > q4ov
> > > > > > r-lkp@...el.com/reproduce)
> > > > > >
> > > > > > If you fix the issue in a separate patch/commit (i.e. not just
> > > > > > a new version of the same patch/commit), kindly add following
> > > > > > tags
> > > > > > | Reported-by: kernel test robot <lkp@...el.com>
> > > > > > | Closes:
> > > > > > | https://lore.kernel.org/oe-kbuild-all/202501292144.eFDq4ovr-
> > > > > > | lkp@
> > > > > > | inte
> > > > > > | l.com/
> > > > > >
> > > > > > sparse warnings: (new ones prefixed by >>)
> > > > > > >> drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:298:38: sparse:
> > > > > > >> sparse: dubious: !x | !y
> > > > > > drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c: note:
> > > > > > in included file
> > > > > (through include/linux/mmzone.h, include/linux/gfp.h,
> > > include/linux/mm.h, ...):
> > > > > > include/linux/page-flags.h:237:46: sparse: sparse:
> > > > > > self-comparison always
> > > > > evaluates to false
> > > > > > include/linux/page-flags.h:237:46: sparse: sparse:
> > > > > > self-comparison always evaluates to false
> > > > > >
> > > > > > vim +298 drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c
> > > > > ...
> > > > > > 297 for (i = 0; i < config->prd_tbl_num; i++) {
> > > > > > > 298 if (!config->sgls[i] | !config->sgls_nent[i])
> > > > > > 299 continue;
> > > > > ...
> > > > >
> > > > > If zeros are unlikely the bit-wise 'or' is pretty likely to
> > > > > generate better code than a logical 'or'.
> > > > >
> > > > > David
> > > >
> > > > Good suggestion!
> > > > So the code can be optimized to:
> > > > for (i = 0; i < config->prd_tbl_num; i++) {
> > > > if (config->sgls[i] && config->sgls_nent[i]) {
> > > > ......
> > > > }
> > >
> > > That just adds a level of indentation to the source.
> > > It'll generate much the same code as !x || !y.
> > > Both will generate two conditional branches.
> > >
> > > It almost certainly makes in immeasurable difference here, but !x |
> > > !y can be generated using (on x86) the 'sete' instruction to get a
> > > 0/1 from the x == 0 compare (that sets the flags), and then doing an 'or' and a
> single jump.
> >
> > You are right.
>
> I'm always right (except when I'm not)
>
> > I do a simple testing:
> >
> > 83 7d f8 00 cmpl $0x0,-0x8(%rbp)
> > 0f 94 c2 sete %dl
> > 83 7d fc 00 cmpl $0x0,-0x4(%rbp)
> > 0f 94 c0 sete %al
>
> On a modern cpu those two pairs will run in parallel.
>
> > 09 d0 or %edx,%eax
> > 84 c0 test %al,%al
>
> The compiler needn't to add the 'test' instruction, but doesn't really like the fact
> that the x86 'sete' is only available for 8-bit registers.
>
> > 74 0f je 1186 <main+0x3d>
> >
> > Compare to:
> >
> > 83 7d f8 00 cmpl $0x0,-0x8(%rbp)
> > 74 06 je 116f <main+0x26>
> > 83 7d fc 00 cmpl $0x0,-0x4(%rbp)
> > 75 0f jne 117e <main+0x35>
> >
> > So you suggest I'd better keep the original (!x | !y) code, if just for unlikely Zero
> comparing?
>
> Looking at the code I'd guess common case is that both values are non-zero.
> So in the normal case both tests are needed.
>
> The branch predictor will (hopefully) get the tests right after the first iteration,
> and I suspect the performance of this code isn't that critical.
>
> So while using | might be an optimisation (rather than a mistake) changing it to ||
> probably doesn't matter.
>
> David
>
Understood.
Thanks for your suggestions and so many details!
Then I prefer to change back to "||" to make it more readable and avoid miss understanding.
Thanks!
Best Regards,
Even
>
> >
> > Thanks!
> >
> > Best Regards,
> > Even Xu
> >
> > >
> > > David
> > >
> > > > }
> > > >
> > > > Will create a patch for this.
> > > >
> > > > Best Regards,
> > > > Even Xu
> > > >
> > > >
> >
Powered by blists - more mailing lists