[systemd-devel] [RFC 7/8] HACK3: minimize the range of D-Cache flush in ioctl(CMD_MSG_RECV)

[AKASHI Takahiro]

On 06/27/2014 01:17 AM, Daniel Mack wrote:
> Hi,
>
> I now booted an ARM board and tried your change, but it doesn't work.
>
> On 06/25/2014 11:13 AM, AKASHI Takahiro wrote:
>>   void kdbus_pool_slice_flush(const struct kdbus_pool_slice *slice)
>>   {
>> +#if KDBUS_HACK3
>> +	flush_kernel_vmap_range((void *)slice->off, slice->size);
>
> slice->off is the offset of the slice inside the shared memory file. It
> will, for instance, be 0 if you happen to access the 1st slice in a
> pool. In your implementation above, you're casting this offset to a
> virtual address, which of course doesn't do the right thing. We need to

Absolutely right :)

> look at the backing shmem file's pages, which is why we need to
> reference them through file->f_mapping. There's no other way to get to
> know which pages to flush the cache for.
>
> The reason why this didn't just crash in your tests is the way
> flush_kernel_vmap_range() is implemented for ARM
> (arch/arm/include/asm/cacheflush.h):
>
> static inline void flush_kernel_vmap_range(void *addr, int size)
> {
>          if ((cache_is_vivt() || cache_is_vipt_aliasing()))
>            __cpuc_flush_dcache_area(addr, (size_t)size);
> }
>
> Your Pandaboard features an OMAP4430 MCU with VIPT/PIPT data cache that
> doesn't suffer from aliasing, and both cache_is_vivt() and
> cache_is_vipt_aliasing() will validate to false on your platform. Hence,
> you effectively made the entire function a no-op. That is correct for an
> OMAP4430, but it will break for anything that has VIVT caches, like the
> PXA3xx SoC I'm testing on.

I think that shmem is primarily used to share data between applications
(user processes), and that no explicit cache operations by users are required
even on VIVT architectures, right?
So it sounds somewhat strange to me that the kernel has to take care of aliasing
on behalf of Apps. (I do understand that flushing is necessary in this specific
use-case of kdbus.)
A matter of design?

> However, flush_dcache_page() flushes the entire dcache for the page,
> which seems to be unnecessary. This is why you see an effect from your
> change. What apparently sufficies is to only flush the dcache aliases,
> and only on platforms where that's necessary. I just don't see a
> portable way of implementing this right now. I'll have to ponder a
> little more over that.

Me neither, but I will think about that.

Thanks,
-Takahiro AKASHI

>
> Thanks,
> Daniel
>