[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: Hardening patch



On Sun, 2011-09-11 at 08:19:42 +0200, Raphael Hertzog wrote:
> On Sun, 11 Sep 2011, Guillem Jover wrote:
> > > +	"bindnow" => 1
> > 
> > Any reason you seem to have ignored the concerns I rised about
> > defaulting to bindnow?
> 
> Well, you mentioned potential performance problems and Kees said
> that his tests did not conclude that it resulted in significant
> performance loss. Kees has been doing the work, I trust him.

I specifically asked on which arches he performed the tests. If he had
said on armel too, then I'd not have any problem with that, but he
didn't reply to that, so I don't see how this is a matter of trust,
when there's just lack of information.

> You told that you would expect the loss to be more significant
> on slower architectures but you did not backup that claim with
> anything.

Well, the concerns were coming from first-hand experience from working
on ARM systems, otherwise I'd not have commented. Specifically on
Maemo the startup time was so bad for UI apps, we created maemo-launcher
just to improve it.

I installed iceweasel on an ARM system (Thecus N2100), w/o X forwarding,
and no user profile, so it just stops when it's not able to find the
DISPLAY, but that should be good enough to get timings close to just the
startup relocation times, which is what the ld.so stats show on amd64
for example. Caches flushed on each iteration, which were pretty
consistent, I've included two different ones for each:

,--- bind lazy ---
# echo 3 > /proc/sys/vm/drop_caches
$ time LD_DEBUG=statistics MOZ_NO_REMOTE=1 iceweasel -ProfileManagea
[...]
Error: no display specified
     17887:
     17887:     runtime linker statistics:
     17887:                final number of relocations: 4233
     17887:     final number of relocations from cache: 29307

real    0m2.279s
user    0m0.310s
sys     0m0.440s

# echo 3 > /proc/sys/vm/drop_caches
$ time LD_DEBUG=statistics MOZ_NO_REMOTE=1 iceweasel -ProfileManager
[...]
Error: no display specified
     17883:
     17883:     runtime linker statistics:
     17883:                final number of relocations: 4233
     17883:     final number of relocations from cache: 29307

real    0m2.187s
user    0m0.320s
sys     0m0.440s
`---

,--- bind now ---
# echo 3 > /proc/sys/vm/drop_caches
$ time LD_BIND_NOW=1 LD_DEBUG=statistics MOZ_NO_REMOTE=1 iceweasel -ProfileManager
[...]
Error: no display specified
     17932:
     17932:     runtime linker statistics:
     17932:                final number of relocations: 19873
     17932:     final number of relocations from cache: 29307

real    0m3.188s
user    0m1.050s
sys     0m0.560s

# echo 3 > /proc/sys/vm/drop_caches
$ time LD_BIND_NOW=1 LD_DEBUG=statistics MOZ_NO_REMOTE=1 iceweasel -ProfileManager
[...]
Error: no display specified
     17924:
     17924:     runtime linker statistics:
     17924:                final number of relocations: 19873
     17924:     final number of relocations from cache: 29307

real    0m3.255s
user    0m1.140s
sys     0m0.490s
`---

As it can bee seen the difference is pretty significant.

> Of course 5% more of 200ms is not the same than 5%
> of 2s but other than that, I do not see any technical reason
> why they would be more affected than i386/amd64.

If it was just a 5% (regardless of the total time) then of course
that'd be ok... And for the technical reasons, well the ELF format
and its ABIs are pretty tied to the architecture and its instruction
set, which influence things like the types of relocations, how the
PLT entries are handled, etc.

> > In any case I don't think enabling this w/o further data demonstrating
> > it's fine to do so is acceptable, as fixing any such regression would
> > imply needing to hunt down packages built with the new flags and trigger
> > binNMUs for them all. The default for it can always be changed later
> > on, I don't see the need to rush it?
> 
> Feel free to change it if you think it's better that way. I'm not attached
> to it.

I'm changing it now on my local tree, will be included in my next
push.

> But I'm not convinced that anyone will do the research that you require on
> non-mainstream architectures and we will just end up with bindnow
> disabled for the indefinite future.

Eh, there's probably more ARM devices in the world than x86 and x86-64
put together. In any case that's how these things are supposed to work,
when someone proposes to change current defaults that might affect the
whole archive, it's on them to provide convincing arguments, and
mitigate raised concerns, not the other way around. If no one is
intersted enough to provide them, well too bad, I guess.

regards,
guillem


Reply to: