Re: [PATCH 01/20] asm-generic/mmiowb: Add generic implementation of mmiowb() tracking

[Nicholas Piggin <npiggin@gmail.com>]

Michael Ellerman's on March 4, 2019 11:01 am:
> Nicholas Piggin <npiggin@gmail.com> writes:
>> Michael Ellerman's on March 3, 2019 7:26 pm:
>>> Nicholas Piggin <npiggin@gmail.com> writes:
> ...
>>>> what was broken about the powerpc one, which is basically:
>>>>
>>>> static inline void mmiowb_set_pending(void)
>>>> {
>>>> 	struct mmiowb_state *ms = __mmiowb_state();
>>>> 	ms->mmiowb_pending = 1;
>>>> }
>>>>
>>>> static inline void mmiowb_spin_lock(void)
>>>> {
>>>> }
>>> 
>>> The current powerpc code clears io_sync in spin_lock().
>>> 
>>> ie, it would be equivalent to:
>>> 
>>> static inline void mmiowb_spin_lock(void)
>>> {
>>>  	ms->mmiowb_pending = 0;
>>> }
>>
>> Ah okay that's what I missed. How about we just not do that?
> 
> Yeah I thought of that too but it's not great. We'd start semi-randomly
> executing the sync in unlock depending on whether someone had done IO on
> that CPU prior to the spinlock.
> 
> eg.
> 
> 	writel(x, y);		// sets paca->io_sync
> 	...	
> 
> 	<schedule>
> 
> 	spin_lock(a);
>         ...
>         // No IO in here
>         ...
>         spin_unlock(a);		// sync() here because other task did writel().
> 
> 
> Which wouldn't be *incorrect*, but would be kind of weird.

schedule is probably okay, we could clear pending there. But you
possibly could get interrupts, or some lock free mmios that set the
flag. Does it matter that much? A random cache miss could have the
same effect.

It may matter slightly less for powerpc because we don't inline
spin locks, although I have been hoping to for a while, this might
put the nail in that.

We can always tinker with it later though so I won't insist.

Thanks,
Nick