[PATCH v2 2/2] powerpc: introduce CONFIG_CACHE_FLUSH_WATCHDOG_THRESHOLD

[Stefan Roese]

On 15.04.21 08:46, Rasmus Villemoes wrote:
> On 15/04/2021 07.34, Stefan Roese wrote:
>> On 13.04.21 21:38, Rasmus Villemoes wrote:
>>>
>>> I have checked with objdump that the generated code doesn't change
>>> when this option is left at its default value of 0: gcc is smart
>>> enough to see that wd_limit is constant 0, the ">=" comparisons are
>>> tautologically true, hence all assignments to "flushed" are eliminated
>>
>> Nitpicking:
>> s/tautologically/autologically.
> 
> No.
> 
> https://www.merriam-webster.com/dictionary/tautological
> https://en.wikipedia.org/wiki/Tautology_(logic)
> "In Mathematical logic, a tautology (from Greek: ταυτολογία) is a
> formula or assertion that is true in every possible interpretation."
> 
> It even exists in gcc docs in the form of -Wtautological-compare. [But I
> do admit that "tautologically true" is a pleonasm; "tautologies" or
> "automatically true" could have been used instead. And going full meta,
> now that I look up pleonasm in wikipedia to check that I'm using it
> right, that article has a reference to "tautology (language)" in its
> second sentence.]

Interesting. Thanks for the detailed info.

>>>        /* wait for all dcbst to complete on bus */
>>>        asm volatile("sync" : : : "memory");
>>> @@ -27,7 +33,11 @@ void flush_cache(ulong start_addr, ulong size)
>>>        for (addr = start; (addr <= end) && (addr >= start);
>>>                addr += CONFIG_SYS_CACHELINE_SIZE) {
>>>            asm volatile("icbi 0,%0" : : "r" (addr) : "memory");
>>> -        WATCHDOG_RESET();
>>> +        flushed += CONFIG_SYS_CACHELINE_SIZE;
>>> +        if (flushed >= wd_limit) {
>>> +            WATCHDOG_RESET();
>>> +            flushed = 0;
>>> +        }
>>
>> It might make sense to pull these 2 identical code snippets into a
>> function to not duplicate this code.
> 
> Something like
> 
> static ulong maybe_reset(ulong flushed)
> {
>    const ulong wd_limit = ...;
>    flushed += CONFIG_SYS_CACHELINE_SIZE;
>    if (flushed >= wd_limit) {
>      WATCHDOG_RESET();
>      flushed = 0;
>    }
>    return flushed;
> }
> 
> and "flushed = maybe_reset(flushed);"? I don't think that improves
> readability.

Yes, something like this. I agree that it's not really beautiful but for
my personal taste it's a bit better. But I don't have hard feeling here.
Let's see what other comment on this.

Thanks,
Stefan