Re: [PATCH v4.16-rc4 1/2] x86/vdso: on Intel, VDSO should handle CLOCK_MONOTONIC_RAW

[Jason Vas Dias]

Good day -

On 12/03/2018, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
> * Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>
>> On Mon, 12 Mar 2018, Jason Vas Dias wrote:
>>
>> checkpatch.pl still reports:
>>
>>    total: 15 errors, 3 warnings, 165 lines checked
>>

Sorry I didn't see you had responded until 40 mins ago .

I finally found where checkpatch.pl is and it now reports :

WARNING: Possible unwrapped commit description (prefer a maximum 75
chars per line)
#2:
--- linux-4.16-rc5.1/arch/x86/entry/vdso/vclock_gettime.c.4.16-rc5	2018-03-12
00:25:09.000000000 +0000

WARNING: struct  should normally be const
#55: FILE: arch/x86/entry/vdso/vclock_gettime.c:282:
+notrace static __always_inline int do_monotonic_raw(struct timespec *ts)


I don't know how to fix that, since 'ts' cannot be a const pointer.


ERROR: Missing Signed-off-by: line(s)


I guess that disappears once someone OKs the patch.

total: 1 errors, 2 warnings, 127 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

../vdso_vclock_gettime_CLOCK_MONOTONIC_RAW-4.16-rc5#1.patch has style
problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


>> > +notrace static u64 vread_tsc_raw(void)
>> > +{
>> > +	u64 tsc, last=gtod->raw_cycle_last;
>> > +	if( likely( gtod->has_rdtscp ) )
>> > +		tsc	= rdtscp((void*)0);
>>
>> Plus I asked more than once to split that rdtscp() stuff into a separate
>> patch.

I misunderstood - I thought you meant the rdtscp implementation
which was split into a separate file - but now it is in a separate patch ,
(attached).

>>
>> You surely are free to ignore my review comments, but rest assured that
>> I'm
>> free to ignore the crap you insist to send me as well.
>

I didn't mean to ignore any comments, and I'm really trying to fix this problem
the right way and not produce crap.


> In addition to Thomas's review feedback I'd strongly urge the careful
> reading of
> Documentation/SubmittingPatches as well:
>
>  - When sending multiple patches please use git-send-mail
>
>  - Please don't send several patch iterations per day!
>
>  - Code quality of the submitted patches is atrocious, please run them
> through
>    scripts/checkpatch.pl (and make sure they pass) to at least enable the
> reading
>    of them.
>
>  - ... plus dozens of other details described in
> Documentation/SubmittingPatches.
>
> Thanks,
>
> 	Ingo
>

I am reading all those documents and cannot see how the code in
the attached patch contravenes any guidelines / best practices -
if you can, please clarify phrases like "atrocious style" - I cannot
see any style guidelines contravened, and I can prove that
the numeric output produced in 16-30ns is just as good
as that produced before the patch was applied in 300-700ns .

Aside from any style comments, any content comments ?

Sorry I am new to latest kernel  guidelines.
I needed to get this problem solved the right way for use at work today.

Thanks for your advice,
Best Regards
Jason

Attachment: vdso_vclock_gettime_CLOCK_MONOTONIC_RAW-4.16-rc5#1.patch
Description: Binary data