Re: [PATCH 1/1] db: disable the ARM assembler mutex code

[Herve Jourdain <herve.jourdain@neuf.fr>]

Hi,

So the issue is whether we want to change the behaviour of previous architectures, or if we try to fix the issue only for the architectures that don't work.
Until now, the db recipe was enabling the 'swp' optimization, and that behavior could be disabled on .bbappend if needed.
While that works fine until armv7ve, it does not work for armv8, which has removed support for those instructions.
Therefore, there is a need to fix it for armv8 - and armv8 only - whereas it can be safely used on previous architectures.
If we remove the use for all ARM architectures, that might create some regression/issues.
If we just remove the use of 'swp' only for armv8, we ensure it doesn't break anything that's running on previous ARM architectures.

Cheers,
Herve

-----Original Message-----
From: Andre McCurdy [mailto:armccurdy@gmail.com] 
Sent: vendredi 15 juin 2018 00:03
To: Khem Raj <raj.khem@gmail.com>
Cc: Herve Jourdain <herve.jourdain@neuf.fr>; Ovidiu Panait <ovidiu.panait@windriver.com>; Patches and discussions about the oe-core layer <openembedded-core@lists.openembedded.org>
Subject: Re: [OE-core] [PATCH 1/1] db: disable the ARM assembler mutex code

On Thu, Jun 14, 2018 at 2:48 PM, Khem Raj <raj.khem@gmail.com> wrote:
> On Thu, Jun 14, 2018 at 1:01 PM Andre McCurdy <armccurdy@gmail.com> wrote:
>> On Thu, Jun 14, 2018 at 12:24 PM, Khem Raj <raj.khem@gmail.com> wrote:
>> > On Thu, Jun 14, 2018 at 12:12 PM Andre McCurdy 
>> > <armccurdy@gmail.com>
>> > wrote:
>> >>
>> >> On Thu, Jun 14, 2018 at 9:40 AM, Khem Raj <raj.khem@gmail.com> wrote:
>> >> > On 6/14/18 5:10 AM, Herve Jourdain wrote:
>> >> >> Hi,
>> >> >>
>> >> >> I believe I solved that same problem by just adding, in the 
>> >> >> case of
>> >> >> armv8
>> >> >> (which I believe may be the new architecture you're referring to):
>> >> >> MUTEX_armv8 = ""
>> >> >> This way, it allows previous versions to work just like they 
>> >> >> did before, without having to disable ARM assembler mutex code 
>> >> >> for architectures that support it correctly - up to armv7ve I 
>> >> >> believe.
>> >> >> Of course, we might need to also have a good definition for 
>> >> >> armv8, which is the object of another thread.
>> >> >
>> >> > right thats a better approach.
>> >>
>> >> SWP is not guaranteed to work on SMP systems... and even if it 
>> >> does, performance is likely to be worse than the pthreads version 
>> >> (which can take advantage of the newer instructions).
>> >>
>> >>
>> >> https://community.arm.com/processors/b/blog/posts/locks-swps-and-t
>> >> wo-smoking-barriers
>> >>
>> >> In general, use of hand optimised assembler mutex implementations 
>> >> in user space isn't something to be encouraged - use pthreads (or 
>> >> maybe a gcc intrinsic) instead.
>> >>
>> >
>> > question is about disabling it on old arm machines, do we have data 
>> > where we know that other sync methods without swp works better on 
>> > armv5 and lower ?
>>
>> On armv5 and below a hand optimised implementation using SWP is 
>> likely to be faster than pthreads. No one is suggesting otherwise.
>>
>> On SMP (highly likely nowadays for armv7 and above), SWP simply might 
>> not work (aside from the fact that if it does work, it's likely to be 
>> slower than pthreads). It's not really a question of performance 
>> there, so the proposal to only disable SWP for armv8 doesn't seem 
>> like a safe solution.
>
> Suggestion is not to just do it for armv8 but To keep it there where 
> its beneficial

You can always argue that micro optimisations are beneficial. The question is whether they make a big enough difference in some real world use case to be worth the maintenance effort.

>> Using pthreads unconditionally is safe and easy. Unless you can prove 
>> that hand optimised SWP is really a big win for armv5 (is anyone 
>> really running a performance critical database on an armv5 system?) 
>> why not keep the recipe simple and use pthreads everywhere?
>>
>> >> I think the original patch is good.
>> >>
>> >> >> Cheers,
>> >> >> Herve
>> >> >>
>> >> >> -----Original Message-----
>> >> >> From: openembedded-core-bounces@lists.openembedded.org
>> >> >> [mailto:openembedded-core-bounces@lists.openembedded.org] On 
>> >> >> Behalf Of Ovidiu Panait
>> >> >> Sent: jeudi 14 juin 2018 13:55
>> >> >> To: openembedded-core@lists.openembedded.org
>> >> >> Subject: [OE-core] [PATCH 1/1] db: disable the ARM assembler 
>> >> >> mutex code
>> >> >>
>> >> >> The swpb in macro MUTEX_SET will cause "undefined instruction" 
>> >> >> error on the new arm arches which don't support this assembly 
>> >> >> instruction any more. If use ldrex/strex to replace swpb, the 
>> >> >> old arm arches don't support them. So to avoid this issue, just 
>> >> >> disable the ARM assembler mutex code, and use the default 
>> >> >> pthreads mutex.
>> >> >>
>> >> >> Signed-off-by: Li Zhou <li.zhou@windriver.com>
>> >> >> Signed-off-by: Catalin Enache <catalin.enache@windriver.com>
>> >> >> Signed-off-by: Ovidiu Panait <ovidiu.panait@windriver.com>
>> >> >> ---
>> >> >>  meta/recipes-support/db/db_5.3.28.bb | 13 +------------
>> >> >>  1 file changed, 1 insertion(+), 12 deletions(-)
>> >> >>
>> >> >> diff --git a/meta/recipes-support/db/db_5.3.28.bb
>> >> >> b/meta/recipes-support/db/db_5.3.28.bb
>> >> >> index 093ee44909..15b4155a29 100644
>> >> >> --- a/meta/recipes-support/db/db_5.3.28.bb
>> >> >> +++ b/meta/recipes-support/db/db_5.3.28.bb
>> >> >> @@ -59,18 +59,7 @@ FILES_SOLIBSDEV = "${libdir}/libdb.so 
>> >> >> ${libdir}/libdb_cxx.so"
>> >> >>  # All the --disable-* options replace --enable-smallbuild, 
>> >> >> which breaks a bunch of stuff (eg. postfix)  DB5_CONFIG ?= 
>> >> >> "--enable-o_direct --disable-cryptography --disable-queue 
>> >> >> --disable-replication --disable-verify --disable-compat185 
>> >> >> --disable-sql"
>> >> >>
>> >> >> -EXTRA_OECONF = "${DB5_CONFIG} --enable-shared --enable-cxx 
>> >> >> --with-sysroot"
>> >> >> -
>> >> >> -# Override the MUTEX setting here, the POSIX library is -# the 
>> >> >> default - "POSIX/pthreads/library".
>> >> >> -# Don't ignore the nice SWP instruction on the ARM:
>> >> >> -# These enable the ARM assembler mutex code, this won't -# 
>> >> >> work with thumb compilation...
>> >> >> -ARM_MUTEX = "--with-mutex=ARM/gcc-assembly"
>> >> >> -MUTEX = ""
>> >> >> -MUTEX_arm = "${ARM_MUTEX}"
>> >> >> -MUTEX_armeb = "${ARM_MUTEX}"
>> >> >> -EXTRA_OECONF += "${MUTEX} STRIP=true"
>> >> >> +EXTRA_OECONF = "${DB5_CONFIG} --enable-shared --enable-cxx
>> >> >> --with-sysroot
>> >> >> STRIP=true"
>> >> >>  EXTRA_OEMAKE += "LIBTOOL='./${HOST_SYS}-libtool'"
>> >> >>
>> >> >>  EXTRA_AUTORECONF += "--exclude=autoheader  -I 
>> >> >> ${S}/dist/aclocal -I${S}/dist/aclocal_java"
>> >> >> --
>> >> >> 2.17.1
>> >> >>
>> >> >> --
>> >> >> _______________________________________________
>> >> >> Openembedded-core mailing list
>> >> >> Openembedded-core@lists.openembedded.org
>> >> >> http://lists.openembedded.org/mailman/listinfo/openembedded-cor
>> >> >> e
>> >> >>
>> >> >
>> >> >
>> >> >
>> >> > --
>> >> > _______________________________________________
>> >> > Openembedded-core mailing list
>> >> > Openembedded-core@lists.openembedded.org
>> >> > http://lists.openembedded.org/mailman/listinfo/openembedded-core
>> >> >