Messages in this thread | | | From | Jason Vas Dias <> | Date | Sun, 18 Mar 2018 03:49:24 +0000 | Subject | Re: [PATCH v4.16-rc5 2/2] x86/vdso: VDSO should handle clock_gettime(CLOCK_MONOTONIC_RAW) without syscall |
| |
On 18/03/2018, Jason Vas Dias <jason.vas.dias@gmail.com> wrote: (should have CC'ed to list, sorry) > On 17/03/2018, Andi Kleen <andi@firstfloor.org> wrote: >> >> That's quite a mischaracterization of the issue. gcc works as intended, >> but the kernel did not correctly supply a indirect call retpoline thunk >> to the vdso, and it just happened to work by accident with the old >> vdso. >> >>> >>> The automated test builds should now succeed with this patch. >> >> How about just adding the thunk function to the vdso object instead of >> this cheap hack? >> >> The other option would be to build vdso with inline thunks. >> >> But just disabling is completely the wrong action. >> >> -Andi >> > > Aha! Thanks for the clarification , Andi! > > I will do so and resend the 2nd patch. > > But is everyone agreed we should accept any slowdown for the timer > functions ? I personally don't think it is a good idea, but I will > regenerate the patch with the thunk function and without > the Makefile change. > > Thanks & Best Regards, > Jason >
I am wondering if it is not better to avoid the thunk being generated and remove the Makefile patch ?
I know that changing the switch in __vdso_clock_gettime() like this avoids the thunk :
switch(clock) { case CLOCK_MONOTONIC: if (do_monotonic(ts) == VCLOCK_NONE) goto fallback; break; default: switch (clock) { case CLOCK_REALTIME: if (do_realtime(ts) == VCLOCK_NONE) goto fallback; break; case CLOCK_MONOTONIC_RAW: if (do_monotonic_raw(ts) == VCLOCK_NONE) goto fallback; break; case CLOCK_REALTIME_COARSE: do_realtime_coarse(ts); break; case CLOCK_MONOTONIC_COARSE: do_monotonic_coarse(ts); break; default: goto fallback; } return 0; fallback: ... }
So at the cost of an unnecessary extra test of the clock parameter, the thunk is avoided .
I wonder if the whole switch should be changed to an if / else clause ?
Or, I know this might be unorthodox, but might work : #define _CAT(V1,V2) V1##V2 #define GTOD_CLK_LABEL(CLK) _CAT(_VCG_L_,CLK) #define MAX_CLK 16 //^^ ?? __vdso_clock_gettime( ... ) { ... static const void *clklbl_tab[MAX_CLK] ={ [ CLOCK_MONOTONIC ] = &>OD_CLK_LABEL(CLOCK_MONOTONIC) , [ CLOCK_MONOTONIC_RAW ] = &>OD_CLK_LABEL(CLOCK_MONOTONIC_RAW) , // and similarly for all clocks handled ... };
goto clklbl_tab[ clock & 0xf ] ;
GTOD_CLK_LABEL(CLOCK_MONOTONIC) : if ( do_monotonic(ts) == VCLOCK_NONE ) goto fallback ;
GTOD_CLK_LABEL(CLOCK_MONOTONIC_RAW) : if ( do_monotonic_raw(ts) == VCLOCK_NONE ) goto fallback ;
... // similarly for all clocks
fallback: return vdso_fallback_gettime(clock,ts); }
If a restructuring like that might be acceptable (with correct tab-based formatting) , and the VDSO can have such a table in its .BSS , I think it would avoid the thunk, and have the advantage of precomputing the jump table at compile-time, and would not require any indirect branches, I think.
Any thoughts ?
Thanks & Best regards, Jason
;
G
| |