#4243 new defect

rtems_cache_coherent_allocate fallback is to return heap memory

Reported by: Chris Johns Owned by:
Priority: normal Milestone: 6.1
Component: lib Version: 6
Severity: normal Keywords:
Cc: Blocked By:
Blocking:

Description

The rtems_cache_coherent_allocate() call in libcsupport uses the malloc heap if a BSP has not added any cache coherent memory to its local specialised heap. As a result memory may or may not be cache coherent.

A call to return cache coherent memory should do so or fail so it is clear what is failing and why.

I do not understand why the call is falling back to memory that is not coherent?

These links explain my position ...

https://git.rtems.org/rtems/tree/cpukit/libcsupport/src/cachecoherentalloc.c#n48
https://git.rtems.org/rtems-libbsd/tree/rtemsbsd/rtems/rtems-kernel-bus-dma.c#n244

The FreeBSD flag BUS_DMA_COHERENT could be redefined as BUS_DMA_COHERENT_MAYBE.

Change History (8)

comment:1 Changed on Feb 15, 2021 at 10:24:46 PM by Chris Johns

I have performed further testing and investigation of PCI memory on a mvme2700 board that has PCI hardware. I added support to the rtemsbsd DMA and mbuf DMA bus buffer handlers to offset segment addresses to the PCI address space as seen by the PCI master. This offset allowed drivers that have a 1:1 physical address mapping to be used unchanged.

I have observed that cache coherent IO_PAGE memory and heap allocated memory behave the same. This testing is limited and may appear to be working but I am not sure and in large applications where there is cache pressures things may break. I think IO_PAGE memory on a PowerPC is required and I will commit the change to add cache coherent memory.

Note, using a cache coherent memory heap may not be compatible with legacy network drivers. Those drivers need to be updated to also use this memory. On the PowerPC the limited number of BATs clashes and the BSP does not know it is linked to LibBSD or the Legacy stack so adding memory cannot be conditional. Legacy driver updates are made more complicated because the i386 is not using the rtemsbsd buf interface in LibBSD and the cache coherent support.

comment:2 Changed on Feb 16, 2021 at 6:10:08 AM by Sebastian Huber

The fall back is correct for systems in which all memory is cache coherent also for DMA. This is the case for all modern PowerPC systems. It is the responsibility of the BSP to provide a cache coherent memory pool if it is needed.

comment:3 in reply to:  2 ; Changed on Feb 16, 2021 at 6:21:59 AM by Chris Johns

Replying to Sebastian Huber:

The fall back is correct for systems in which all memory is cache coherent also for DMA.

It is not correct for a call of this type to make any assumption other than returning memory from the pool it has been given.

This is the case for all modern PowerPC systems. It is the responsibility of the BSP to provide a cache coherent memory pool if it is needed.

If a BSP has a specific ability to do this then why not provide some memory from the malloc heap to the cache coherent allocator? I see not doing this as a bug in those BSPs that needs to be fixed. I do not agree with hiding something like this in a call as it currently is. A call in a BSP makes it easy to determine the intended mode of operation.

I hope you understand that have I tripped over this and I will not be the last. Generating an error makes it easy to see you need to sort this issue out in a BSP.

comment:4 in reply to:  3 ; Changed on Feb 16, 2021 at 6:31:44 AM by Sebastian Huber

Replying to Chris Johns:

Replying to Sebastian Huber:

The fall back is correct for systems in which all memory is cache coherent also for DMA.

It is not correct for a call of this type to make any assumption other than returning memory from the pool it has been given.

It is the responsibility of the BSP to set up this allocator. There are a couple of BSP for which this fall back is absolutely the right thing to do.

This is the case for all modern PowerPC systems. It is the responsibility of the BSP to provide a cache coherent memory pool if it is needed.

If a BSP has a specific ability to do this then why not provide some memory from the malloc heap to the cache coherent allocator?

The BSP doesn't know how much memory it needs to allocate from malloc for this memory.

I see not doing this as a bug in those BSPs that needs to be fixed. I do not agree with hiding something like this in a call as it currently is. A call in a BSP makes it easy to determine the intended mode of operation.

I hope you understand that have I tripped over this and I will not be the last. Generating an error makes it easy to see you need to sort this issue out in a BSP.

This is understandable. From my point of view this is a documentation issue. Maybe we need a BSP checklist, e.g. if your system has no cache/DMA coherent memory, then make sure the cache coherent memory allocator is set up.

comment:5 in reply to:  4 ; Changed on Feb 16, 2021 at 7:02:37 AM by Chris Johns

Replying to Sebastian Huber:

Replying to Chris Johns:

Replying to Sebastian Huber:

The fall back is correct for systems in which all memory is cache coherent also for DMA.

It is not correct for a call of this type to make any assumption other than returning memory from the pool it has been given.

It is the responsibility of the BSP to set up this allocator.

Agreed, so ....

There are a couple of BSP for which this fall back is absolutely the right thing to do.

... why not have those couple of BSPs take responsibility like all other BSPs and set up the allocator?

This is the case for all modern PowerPC systems. It is the responsibility of the BSP to provide a cache coherent memory pool if it is needed.

If a BSP has a specific ability to do this then why not provide some memory from the malloc heap to the cache coherent allocator?

The BSP doesn't know how much memory it needs to allocate from malloc for this memory.

Yes the motorola_powerpc has the same issue so nothing different here either. And I see it as being no different to other system level sizes we are forced to set.

I see not doing this as a bug in those BSPs that needs to be fixed. I do not agree with hiding something like this in a call as it currently is. A call in a BSP makes it easy to determine the intended mode of operation.

I hope you understand that have I tripped over this and I will not be the last. Generating an error makes it easy to see you need to sort this issue out in a BSP.

This is understandable. From my point of view this is a documentation issue. Maybe we need a BSP checklist, e.g. if your system has no cache/DMA coherent memory, then make sure the cache coherent memory allocator is set up.

The fundamental issue is the name of the function does not match what is does and it is inconsistent in terms of an API function. I expect a specially named allocator to do what it says or fail without making any assumptions.

I would accept the call being rtems_cache_coherent_allocate_if_set_up_else_just_memory() but I think it is too long. :)

comment:6 Changed on Feb 16, 2021 at 8:32:05 PM by Chris Johns <chrisj@…>

In 026eb3db/rtems:

powerpc/motorola_powerpc: Add cache coherent memory to the allocator

Updates #4245
Updates #4243

comment:7 in reply to:  5 ; Changed on Feb 17, 2021 at 5:41:12 AM by Sebastian Huber

Replying to Chris Johns:

Replying to Sebastian Huber:

Replying to Chris Johns:

Replying to Sebastian Huber:

The fall back is correct for systems in which all memory is cache coherent also for DMA.

It is not correct for a call of this type to make any assumption other than returning memory from the pool it has been given.

It is the responsibility of the BSP to set up this allocator.

Agreed, so ....

There are a couple of BSP for which this fall back is absolutely the right thing to do.

... why not have those couple of BSPs take responsibility like all other BSPs and set up the allocator?

The function to simply use the heap on systems where this makes sense should be preserved. If you want an explicit initialization for this case, then please add it.

comment:8 in reply to:  7 Changed on Feb 22, 2021 at 10:14:53 PM by Chris Johns

Replying to Sebastian Huber:

Replying to Chris Johns:

Replying to Sebastian Huber:

Replying to Chris Johns:

Replying to Sebastian Huber:

The fall back is correct for systems in which all memory is cache coherent also for DMA.

It is not correct for a call of this type to make any assumption other than returning memory from the pool it has been given.

It is the responsibility of the BSP to set up this allocator.

Agreed, so ....

There are a couple of BSP for which this fall back is absolutely the right thing to do.

... why not have those couple of BSPs take responsibility like all other BSPs and set up the allocator?

The function to simply use the heap on systems where this makes sense should be preserved.

It appears to me this is a convenience for a niche set of BSPs. Over the life of RTEMS I have found special case like this at best provide surprises and at worst make long term maintenance harder. For example as the "special" case BSPs age the ability to make changes and test becomes harder and in the end we cannot remove this special case if we wanted too.

The rational is simple, make the call do just what the name suggests it does and make it consistent. The call is to provide cache coherent memory and to fail if there is none. Having it assume a default mode may work for some but fails for others. My key point is the failure for some is more important than the convenience of others.

If you want an explicit initialization for this case, then please add it.

I will be providing a patch in time to remove the malloc heap fall back. However I do not have or know the list of BSPs where malloc heap memory is OK so I cannot updated them. That alone should signal concern about this call.

If you have another solution in mind that meets the requirements I have stated I suggest you post a patch. As you say a call to force the allocator would be simple and one I consider as OK. I consider it OK because a simple grep lets me find which BSPs can support this. Everyone else fails until they are updated, something I am happy to see happen.

Note: See TracTickets for help on using tickets.