Re: [patch V4 part 2 10/18] x86/entry/64: Check IF in __preempt_enable_notrace() thunk

[Thomas Gleixner]

Thomas Gleixner <tglx@xxxxxxxxxxxxx> writes:
> Andy Lutomirski <luto@xxxxxxxxxx> writes:
>> On Tue, May 5, 2020 at 7:14 AM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>> /**
>>  * preempt_schedule_notrace - preempt_schedule called by tracing
>>  *
>>  * The tracing infrastructure uses preempt_enable_notrace to prevent
>>  * recursion and tracing preempt enabling caused by the tracing
>>  * infrastructure itself. But as tracing can happen in areas coming
>>  * from userspace or just about to enter userspace, a preempt enable
>>  * can occur before user_exit() is called. This will cause the scheduler
>>  * to be called when the system is still in usermode.
>>  *
>>  * To prevent this, the preempt_enable_notrace will use this function
>>  * instead of preempt_schedule() to exit user context if needed before
>>  * calling the scheduler.
>>  */
>>
>> Which is no longer really applicable to x86 -- in the state that this
>> comment nonsensically refers to as "userspace", x86 *always* has IRQs
>> off, which means that preempt_enable() will not schedule.

Yeah.

>> So I'm guessing that the issue you're solving is that we have
>> redundant preempt disable/enable pairs somewhere in the bowels of
>> tracing code that is called with IRQs off, and objtool is now
>> complaining.  Could the actual code in question be fixed to assert
>> that IRQs are off instead of disabling preemption?  If not, can you
>> fix the $SUBJECT and changelog and perhaps add a comment to the code
>> as to *why* you're checking IF?  Otherwise some intrepid programmer is
>> going to notice it down the road, wonder if it's optimizing anything
>> useful at all, and get rid of it.
>
> Let me stare into that again.

There are a few preempt_disable/enable() pairs in some of the helper
functions which are called in various places. That means we would have
to chase all of them and provide 'naked' helpers for these particular
call chains. I'll fix the changelog and add a comment to make clear what
this is about.

Thanks,

        tglx