lkml.org 
[lkml]   [2018]   [Mar]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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 ]
= &&GTOD_CLK_LABEL(CLOCK_MONOTONIC) ,
[ CLOCK_MONOTONIC_RAW ]
= &&GTOD_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

\
 
 \ /
  Last update: 2018-03-18 04:50    [W:0.062 / U:0.212 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site