linux-kernel - Re: Commit 34d76c41 causes linker errors on ia64 with NR

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 27 Oct 2009 11:03:24 +0100 (CET)
From:	Jiri Kosina <jkosina@...e.cz>
To:	Ingo Molnar <mingo@...e.hu>
Cc:	Jeff Mahoney <jeffm@...e.com>, Tejun Heo <tj@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Tony Luck <tony.luck@...el.com>,
	Fenghua Yu <fenghua.yu@...el.com>, linux-ia64@...r.kernel.org
Subject: Re: Commit 34d76c41 causes linker errors on ia64 with NR_CPUS=4096

On Mon, 26 Oct 2009, Ingo Molnar wrote:

> > -struct update_shares_data {
> > -	unsigned long rq_weight[NR_CPUS];
> > -};
> > -
> > -static DEFINE_PER_CPU(struct update_shares_data, update_shares_data);
> > +unsigned long *update_shares_data;
> 
> That should be __read_mostly - this can be a hotly accessed data 
> structure of the scheduler - if it happens to go next to a frequently 
> bouncing variable that can be bad for performance.

Good point, will resend with this annotation added.

> > -				    struct update_shares_data *usd)
> > +				    unsigned long *usd)
> 
> I dont think using usd[cpu] is clearer than usd->rq_weight[cpu]. At 
> minimum it should be renamed to usd_rq_weight not usd.

OK.

> >  	local_irq_save(flags);
> > -	usd = &__get_cpu_var(update_shares_data);
> > +	usd = per_cpu_ptr(update_shares_data, smp_processor_id());
> 
> Could we please have a look at the before/after assembly of this 
> sequence on x86, to make sure the claims in this thread are true and we 
> dont lose performance? (and included it in the changelog with a 
> resubmission - with a new, changed '[PATCH] ...' subject line, not 
> hidden inside a discussion thread.)

Just for completness (I will include it in the changelog with the next 
resubmission shortly):


Before:

...
ffffffff8104337c:       65 48 8b 14 25 20 cd    mov    %gs:0xcd20,%rdx
ffffffff81043383:       00 00
ffffffff81043385:       48 c7 c0 00 e1 00 00    mov    $0xe100,%rax
ffffffff8104338c:       48 c7 45 a0 00 00 00    movq   $0x0,-0x60(%rbp)
ffffffff81043393:       00
ffffffff81043394:       48 c7 45 a8 00 00 00    movq   $0x0,-0x58(%rbp)
ffffffff8104339b:       00
ffffffff8104339c:       48 01 d0                add    %rdx,%rax
ffffffff8104339f:       49 8d 94 24 08 01 00    lea    0x108(%r12),%rdx
ffffffff810433a6:       00
ffffffff810433a7:       b9 ff ff ff ff          mov    $0xffffffff,%ecx
ffffffff810433ac:       48 89 45 b0             mov    %rax,-0x50(%rbp)
ffffffff810433b0:       bb 00 04 00 00          mov    $0x400,%ebx
ffffffff810433b5:       48 89 55 c0             mov    %rdx,-0x40(%rbp)
...

After:

...
ffffffff8104337c:       65 8b 04 25 28 cd 00    mov    %gs:0xcd28,%eax
ffffffff81043383:       00
ffffffff81043384:       48 98                   cltq
ffffffff81043386:       49 8d bc 24 08 01 00    lea    0x108(%r12),%rdi
ffffffff8104338d:       00
ffffffff8104338e:       48 8b 15 d3 7f 76 00    mov    0x767fd3(%rip),%rdx        # ffffffff817ab368 <update_shares_data>
ffffffff81043395:       48 8b 34 c5 00 ee 6d    mov    -0x7e921200(,%rax,8),%rsi
ffffffff8104339c:       81
ffffffff8104339d:       48 c7 45 a0 00 00 00    movq   $0x0,-0x60(%rbp)
ffffffff810433a4:       00
ffffffff810433a5:       b9 ff ff ff ff          mov    $0xffffffff,%ecx
ffffffff810433aa:       48 89 7d c0             mov    %rdi,-0x40(%rbp)
ffffffff810433ae:       48 c7 45 a8 00 00 00    movq   $0x0,-0x58(%rbp)
ffffffff810433b5:       00
ffffffff810433b6:       bb 00 04 00 00          mov    $0x400,%ebx
ffffffff810433bb:       48 01 f2                add    %rsi,%rdx
ffffffff810433be:       48 89 55 b0             mov    %rdx,-0x50(%rbp)
...

> From a merge POV i'm quite nervous about such a change to the scheduler 
> this late in the .32 cycle - to offset that risk i'd really like to see 
> that this change has been pursued carefully to the edge of possibilities 
> - currently it does not give that impression.

So what's your other proposal?

Refactoring the ia64 pagefault handler is no-go this late in the cycle 
IMHO.

So either this, or reverting 34d76c41 (which is also no-go, I believe).

-- 
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/