Re: [PATCH v4.16-rc5 2/2] x86/vdso: VDSO should handle clock_gettime(CLOCK_MONOTONIC_RAW) without syscall

[Jason Vas Dias]

On 18/03/2018, Jason Vas Dias <jason.vas.dias@xxxxxxxxx> wrote:
(should have CC'ed to list, sorry)
> On 17/03/2018, Andi Kleen <andi@xxxxxxxxxxxxxx> 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