[PATCH 1/4] arm: Fix armv7 neon memchr on ARM mode

classic Classic list List threaded Threaded
26 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Wilco Dijkstra-2
Phil Blundell wrote:

> But here we are, two decades later, and the landscape is a bit
> different.  It's not entirely clear that -marm/-mthumb are very useful
> options in an ARMv7 world because the user shouldn't need to care.  In
> an ideal world the compiler would be able to predict for any given
> function whether T32 or A32 encoding would give the best results
> (either speed or space according to the selected optimisation settings)
> and proceed accordingly.  Unfortunately in practice I don't think
> we're quite there yet and for critical code there's still an element of
> "try building it both ways round and see which one runs quickest" which
> means the compiler flags probably are still necessary.  Users might
> even legitimately wish to try that experiment with glibc, so forbidding
> them to compile it under -marm would not obviously be the right thing
> to do.

If there are still cases where GCC generates worse code for Thumb-2,
then those can be addressed easily. Our very first Thumb-2 compiler
had performance within 2% on ARM1156T2 more than 2 decades ago!

Things have improved a lot since then, and system wide effects of code
footprint have become more important, so using only Arm on a modern
core just doesn't make any sense today.

Wilco
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Florian Weimer-5
On 04/13/2018 05:43 PM, Wilco Dijkstra wrote:
> Things have improved a lot since then, and system wide effects of code
> footprint have become more important, so using only Arm on a modern
> core just doesn't make any sense today.

We increasingly see on other architectures that people turn of
configurable kernel features, maybe as some form of security hardening
(threat surface reduction).  This means that code which assumes the
presence of these kernel features (because the used to be non-optional
and may even be mentioned in ABI documents) no longer runs.
Consequently, you have to convince others to change their kernel
configuration, or you have to build your software differently.

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Adhemerval Zanella-2
In reply to this post by Phil Blundell-3


On 13/04/2018 12:07, Phil Blundell wrote:

> On Fri, 2018-04-13 at 11:40 -0300, Adhemerval Zanella wrote:
>> I am not against reverting it, but it does not help the scenario of
>> BZ#23031.  We need to define whether environments which thumb is
>> expected to be supported but it is not enabled by compiler flags
>> (-marm) and act accordingly by avoid building glibc in such cases.
>
> My position on that remains that the scenario of BZ#23031 is simply
> invalid and the user's expectations are just misplaced.  The fact that
> we've had sysdeps/arm/armv6t2/memchr.S using Thumb code since December
> 2011 and in those six years it's only generated one bug report seems to
> support the belief that there is not a large population of users trying
> to do this sort of thing.

My understanding is glibc try support kernel with missing functionalities
but either using fallback implementations (either subpar with underlying
issues as for old pipe2), emulation (as for copy_file_range which tries
to emulate the kernel behaviour), or by just warning userland the kernel
do not provide the underlying support (for instance set_robust_list).

I really think just saying 'go fix your kernel' is not the correct answer,
even more when the configuration used is supported upstream (it not an
experimental or out-of-tree one).  Also for specific case, my wild guess
is using ARM code path should not shown in much difference and will
prevent such possible issues.

>
> I don't think -marm has ever been intended to mean "don't allow any
> Thumb instructions in my program".  The whole -marm/-mthumb thing dates
> from ARMv4T/Thumb-1 days when interworking couldn't be taken for
> granted and Thumb code often ran significantly slower than the
> equivalent ARM code.  Under those circumstances it was necessary for
> the user to be able to choose what instruction encoding the compiler
> was going to generate.
>
> But here we are, two decades later, and the landscape is a bit
> different.  It's not entirely clear that -marm/-mthumb are very useful
> options in an ARMv7 world because the user shouldn't need to care.  In
> an ideal world the compiler would be able to predict for any given
> function whether T32 or A32 encoding would give the best results
> (either speed or space according to the selected optimisation settings)
>  and proceed accordingly.  Unfortunately in practice I don't think
> we're quite there yet and for critical code there's still an element of
> "try building it both ways round and see which one runs quickest" which
> means the compiler flags probably are still necessary.  Users might
> even legitimately wish to try that experiment with glibc, so forbidding
> them to compile it under -marm would not obviously be the right thing
> to do.
>
> p.
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Phil Blundell-3
On Fri, 2018-04-13 at 13:42 -0300, Adhemerval Zanella wrote:
> My understanding is glibc try support kernel with missing
> functionalities but either using fallback implementations (either
> subpar with underlying issues as for old pipe2), emulation (as for
> copy_file_range which tries to emulate the kernel behaviour), or by
> just warning userland the kernel do not provide the underlying
> support (for instance set_robust_list).

It's true that glibc has tried (with varying levels of success) to
provide compatibility implementations so that new APIs can be used on
older kernels which lack some or other functionality.  But this doesn't
extend to working around arbitrarily broken kernel configurations.

The cost of enabling CONFIG_ARM_THUMB in your kernel if you're building
for ARMv7 anyway is so close to zero as to be completely negligible.
There is simply no rational reason to not have it included.  It would
be interesting to ask the guy who filed that bugzilla ticket whether he
has turned it off deliberately, and if so why.  I suspect he probably
just didn't realise it was needed, rather than actively wanting it off.

> I really think just saying 'go fix your kernel' is not the correct
> answer, even more when the configuration used is supported upstream
> (it not an experimental or out-of-tree one). 

There are any number of other ways in which you can break your system
by configuring your kernel wrongly.  The fact that the kernel config
system lets you turn a given option on or off doesn't mean you can just
flip switches at random and expect things to work.  That said, in the
particular case of CONFIG_ARM_THUMB, I think the kernel people should
simply force this on when CONFIG_CPU_V7 is selected.

> Also for specific case, my wild guess is using ARM code path should
> not shown in much difference and will prevent such possible issues.

In this particular case you're right, the ARM implementation is
probably not going to perform very much worse, and now that you've
fixed at least the obvious bugs I think it will probably work fine.  

My concern is about the precedent it sets for the future.  Right now,
it clearly is not true that building glibc for ARMv7 with -marm will
use only A32 instructions.  Further, as far as I can tell it has never
been true, so there cannot be any existing users who are depending on
this behaviour.  If we fix this bug in the way that you are proposing,
we would be making an implicit promise that any future ARMv7-optimised
assembly code is also sensitive to being compiled under -marm and will
avoid Thumb2 instructions in that situation.

So, all in all it seems we have a choice between:

- we just tell this one guy "sorry, you have to enable CONFIG_ARM_THUMB
in your kernel for glibc to work on ARMv7", he enables the option, it
doesn't cost him anything, and everyone moves on; or

- we fix glibc to avoid Thumb2 instructions in these assembly files,
which means we now have an extra variant that we have to maintain and
test, we need to remember to add the same checks to any future assembly
code that might be added for v7 in the future, and we essentially can't
ever stop doing that for fear that users might have become dependent on
it

I would prefer to do the former.

p.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Adhemerval Zanella-2


On 13/04/2018 16:09, Phil Blundell wrote:

> On Fri, 2018-04-13 at 13:42 -0300, Adhemerval Zanella wrote:
>> My understanding is glibc try support kernel with missing
>> functionalities but either using fallback implementations (either
>> subpar with underlying issues as for old pipe2), emulation (as for
>> copy_file_range which tries to emulate the kernel behaviour), or by
>> just warning userland the kernel do not provide the underlying
>> support (for instance set_robust_list).
>
> It's true that glibc has tried (with varying levels of success) to
> provide compatibility implementations so that new APIs can be used on
> older kernels which lack some or other functionality.  But this doesn't
> extend to working around arbitrarily broken kernel configurations.
>
> The cost of enabling CONFIG_ARM_THUMB in your kernel if you're building
> for ARMv7 anyway is so close to zero as to be completely negligible.
> There is simply no rational reason to not have it included.  It would
> be interesting to ask the guy who filed that bugzilla ticket whether he
> has turned it off deliberately, and if so why.  I suspect he probably
> just didn't realise it was needed, rather than actively wanting it off.

Fair enough, looks like CONFIG_ARM_THUMB is not defined only for some
old armv5t kernel config.

>
>> I really think just saying 'go fix your kernel' is not the correct
>> answer, even more when the configuration used is supported upstream
>> (it not an experimental or out-of-tree one). 
>
> There are any number of other ways in which you can break your system
> by configuring your kernel wrongly.  The fact that the kernel config
> system lets you turn a given option on or off doesn't mean you can just
> flip switches at random and expect things to work.  That said, in the
> particular case of CONFIG_ARM_THUMB, I think the kernel people should
> simply force this on when CONFIG_CPU_V7 is selected.

I strong agree CONFIG_CPU_V7 should imply CONFIG_ARM_THUMB.

>
>> Also for specific case, my wild guess is using ARM code path should
>> not shown in much difference and will prevent such possible issues.
>
> In this particular case you're right, the ARM implementation is
> probably not going to perform very much worse, and now that you've
> fixed at least the obvious bugs I think it will probably work fine.  
>
> My concern is about the precedent it sets for the future.  Right now,
> it clearly is not true that building glibc for ARMv7 with -marm will
> use only A32 instructions.  Further, as far as I can tell it has never
> been true, so there cannot be any existing users who are depending on
> this behaviour.  If we fix this bug in the way that you are proposing,
> we would be making an implicit promise that any future ARMv7-optimised
> assembly code is also sensitive to being compiled under -marm and will
> avoid Thumb2 instructions in that situation.
>
> So, all in all it seems we have a choice between:
>
> - we just tell this one guy "sorry, you have to enable CONFIG_ARM_THUMB
> in your kernel for glibc to work on ARMv7", he enables the option, it
> doesn't cost him anything, and everyone moves on; or
>
> - we fix glibc to avoid Thumb2 instructions in these assembly files,
> which means we now have an extra variant that we have to maintain and
> test, we need to remember to add the same checks to any future assembly
> code that might be added for v7 in the future, and we essentially can't
> ever stop doing that for fear that users might have become dependent on
> it
>
> I would prefer to do the former.
>
> p.
>

I do prefer the former and this discussion indeed shown there is little
gain in maintaining two build modes for the string functions.  I will
resend the patch with just removing the ARM code path and make thumb
as default an only build option.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

Richard Earnshaw (lists)
In reply to this post by Phil Blundell-3
On 13/04/18 20:09, Phil Blundell wrote:

> On Fri, 2018-04-13 at 13:42 -0300, Adhemerval Zanella wrote:
>> My understanding is glibc try support kernel with missing
>> functionalities but either using fallback implementations (either
>> subpar with underlying issues as for old pipe2), emulation (as for
>> copy_file_range which tries to emulate the kernel behaviour), or by
>> just warning userland the kernel do not provide the underlying
>> support (for instance set_robust_list).
>
> It's true that glibc has tried (with varying levels of success) to
> provide compatibility implementations so that new APIs can be used on
> older kernels which lack some or other functionality.  But this doesn't
> extend to working around arbitrarily broken kernel configurations.
>
> The cost of enabling CONFIG_ARM_THUMB in your kernel if you're building
> for ARMv7 anyway is so close to zero as to be completely negligible.
> There is simply no rational reason to not have it included.  It would
> be interesting to ask the guy who filed that bugzilla ticket whether he
> has turned it off deliberately, and if so why.  I suspect he probably
> just didn't realise it was needed, rather than actively wanting it off.
>
>> I really think just saying 'go fix your kernel' is not the correct
>> answer, even more when the configuration used is supported upstream
>> (it not an experimental or out-of-tree one). 
>
> There are any number of other ways in which you can break your system
> by configuring your kernel wrongly.  The fact that the kernel config
> system lets you turn a given option on or off doesn't mean you can just
> flip switches at random and expect things to work.  That said, in the
> particular case of CONFIG_ARM_THUMB, I think the kernel people should
> simply force this on when CONFIG_CPU_V7 is selected.
>
>> Also for specific case, my wild guess is using ARM code path should
>> not shown in much difference and will prevent such possible issues.
>
> In this particular case you're right, the ARM implementation is
> probably not going to perform very much worse, and now that you've
> fixed at least the obvious bugs I think it will probably work fine.  
>
> My concern is about the precedent it sets for the future.  Right now,
> it clearly is not true that building glibc for ARMv7 with -marm will
> use only A32 instructions.  Further, as far as I can tell it has never
> been true, so there cannot be any existing users who are depending on
> this behaviour.  If we fix this bug in the way that you are proposing,
> we would be making an implicit promise that any future ARMv7-optimised
> assembly code is also sensitive to being compiled under -marm and will
> avoid Thumb2 instructions in that situation.
>
> So, all in all it seems we have a choice between:
>
> - we just tell this one guy "sorry, you have to enable CONFIG_ARM_THUMB
> in your kernel for glibc to work on ARMv7", he enables the option, it
> doesn't cost him anything, and everyone moves on; or
>

+1 for all of the above :-)

> - we fix glibc to avoid Thumb2 instructions in these assembly files,
> which means we now have an extra variant that we have to maintain and
> test, we need to remember to add the same checks to any future assembly
> code that might be added for v7 in the future, and we essentially can't
> ever stop doing that for fear that users might have become dependent on
> it
>
> I would prefer to do the former.

It takes a lot of work to tune the routines written in assembler across
a range of processors.  Needlessly repeating that for perverse kernel
configurations is just make work.



R.
12