| Message ID | YhToFzlSufrliUsi@MiWiFi-R3L-srv (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/2] dma-mapping: check dma_mask for streaming mapping allocs | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Tue, Feb 22, 2022 at 09:41:43PM +0800, Baoquan He wrote: > For newly added streaming mapping APIs, the internal core function > __dma_alloc_pages() should check dev->dma_mask, but not > ev->coherent_dma_mask which is for coherent mapping. No, this is wrong. dev->coherent_dma_mask is and should be used here. > > > Meanwhile, just filter out gfp flags if they are any of > __GFP_DMA, __GFP_DMA32 and __GFP_HIGHMEM, but not fail it. This change > makes it consistent with coherent mapping allocs. This is wrong as well. We want to eventually fail dma_alloc_coherent for these, too. It just needs more work.
On 02/22/22 at 04:59pm, Christoph Hellwig wrote: > On Tue, Feb 22, 2022 at 09:41:43PM +0800, Baoquan He wrote: > > For newly added streaming mapping APIs, the internal core function > > __dma_alloc_pages() should check dev->dma_mask, but not > > ev->coherent_dma_mask which is for coherent mapping. > > No, this is wrong. dev->coherent_dma_mask is and should be used here. Could you tell more why this is wrong? According to Documentation/core-api/dma-api.rst and DMA code, __dma_alloc_pages() is the core function of dma_alloc_pages()/dma_alloc_noncoherent() which are obviously streaming mapping, why do we need to check dev->coherent_dma_mask here? Because dev->coherent_dma_mask is the subset of dev->dma_mask, it's safer to use dev->coherent_dma_mask in these places? This is confusing, I talked to Hyeonggon in private mail, he has the same feeling. > > > > > > > Meanwhile, just filter out gfp flags if they are any of > > __GFP_DMA, __GFP_DMA32 and __GFP_HIGHMEM, but not fail it. This change > > makes it consistent with coherent mapping allocs. > > This is wrong as well. We want to eventually fail dma_alloc_coherent > for these, too. It just needs more work. >
On Wed, Feb 23, 2022 at 08:28:13AM +0800, Baoquan He wrote: > Could you tell more why this is wrong? According to > Documentation/core-api/dma-api.rst and DMA code, __dma_alloc_pages() is > the core function of dma_alloc_pages()/dma_alloc_noncoherent() which are > obviously streaming mapping, Why are they "obviously" streaming mappings? > why do we need to check > dev->coherent_dma_mask here? Because dev->coherent_dma_mask is the subset > of dev->dma_mask, it's safer to use dev->coherent_dma_mask in these > places? This is confusing, I talked to Hyeonggon in private mail, he has > the same feeling. Think of th coherent_dma_mask as dma_alloc_mask. It is the mask for the DMA memory allocator. dma_mask is the mask for the dma_map_* routines.
From: Christoph Hellwig > Sent: 23 February 2022 14:26 > > On Wed, Feb 23, 2022 at 08:28:13AM +0800, Baoquan He wrote: > > Could you tell more why this is wrong? According to > > Documentation/core-api/dma-api.rst and DMA code, __dma_alloc_pages() is > > the core function of dma_alloc_pages()/dma_alloc_noncoherent() which are > > obviously streaming mapping, > > Why are they "obviously" streaming mappings? > > > why do we need to check > > dev->coherent_dma_mask here? Because dev->coherent_dma_mask is the subset > > of dev->dma_mask, it's safer to use dev->coherent_dma_mask in these > > places? This is confusing, I talked to Hyeonggon in private mail, he has > > the same feeling. > > Think of th coherent_dma_mask as dma_alloc_mask. It is the mask for the > DMA memory allocator. dma_mask is the mask for the dma_map_* routines. I suspect it is all to allow for things like: - A 64bit system with memory above 4G. - A device that can only generate 32bit addresses. - Some feature of the memory system (or bus bridges) that restricts cache snooping to the low 1G of address space. So dma_alloc_coherent() has to allocate memory below 1G. The dma_map functions have to use bounce-buffers for addresses above 4G. dma_alloc_noncoherent() can allocate anything below 4G and so avoid bounce buffers later on. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 02/23/22 at 03:25pm, Christoph Hellwig wrote: > On Wed, Feb 23, 2022 at 08:28:13AM +0800, Baoquan He wrote: > > Could you tell more why this is wrong? According to > > Documentation/core-api/dma-api.rst and DMA code, __dma_alloc_pages() is > > the core function of dma_alloc_pages()/dma_alloc_noncoherent() which are > > obviously streaming mapping, > > Why are they "obviously" streaming mappings? Because they are obviously not coherent mapping? With my understanding, there are two kinds of DMA mapping, coherent mapping (which is also persistent mapping), and streaming mapping. The coherent mapping will be handled during driver init, and released during driver de-init. While streaming mapping will be done when needed at any time, and released after usage. Are we going to add another kind of mapping? It's not streaming mapping, but use dev->coherent_dma_mask, just because it uses dma_alloc_xxx() api. > > > why do we need to check > > dev->coherent_dma_mask here? Because dev->coherent_dma_mask is the subset > > of dev->dma_mask, it's safer to use dev->coherent_dma_mask in these > > places? This is confusing, I talked to Hyeonggon in private mail, he has > > the same feeling. > > Think of th coherent_dma_mask as dma_alloc_mask. It is the mask for the > DMA memory allocator. dma_mask is the mask for the dma_map_* routines. I will check code further. While this may need be noted in doc, e.g dma_api.rst or dma-api-howto.rst. If you have guide, I can try to add some words to make clear this. Or leave this to people who knows this clearly. I believe it will be very helpful to understand DMA api.
From: Baoquan He > Sent: 24 February 2022 14:11 ... > With my understanding, there are two kinds of DMA mapping, coherent > mapping (which is also persistent mapping), and streaming mapping. The > coherent mapping will be handled during driver init, and released during > driver de-init. While streaming mapping will be done when needed at any > time, and released after usage. The lifetime has absolutely nothing to do with it. It is all about how the DMA cycles (from the device) interact with (or more don't interact with) the cpu memory cache. For coherent mapping the cpu and device can write to (different) words in the same cache line at the same time, and both will see both updates. On some systems this can only be achieved by making the memory uncached - which significantly slows down cpu access. For non-coherent (streaming) mapping the cpu writes back and/or invalidates the data cache so that the dma read cycles from memory read the correct data and the cpu re-reads the cache line after the dma has completed. They are only really suitable for data buffers. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 02/24/22 at 02:27pm, David Laight wrote: > From: Baoquan He > > Sent: 24 February 2022 14:11 > ... > > With my understanding, there are two kinds of DMA mapping, coherent > > mapping (which is also persistent mapping), and streaming mapping. The > > coherent mapping will be handled during driver init, and released during > > driver de-init. While streaming mapping will be done when needed at any > > time, and released after usage. > > The lifetime has absolutely nothing to do with it. > > It is all about how the DMA cycles (from the device) interact with > (or more don't interact with) the cpu memory cache. > > For coherent mapping the cpu and device can write to (different) > words in the same cache line at the same time, and both will see > both updates. > On some systems this can only be achieved by making the memory > uncached - which significantly slows down cpu access. > > For non-coherent (streaming) mapping the cpu writes back and/or > invalidates the data cache so that the dma read cycles from memory > read the correct data and the cpu re-reads the cache line after > the dma has completed. > They are only really suitable for data buffers. Thanks for valuable input, I agree the lifetime is not stuff we can rely on to judge. But how do we explain dma_alloc_noncoherent() is not streaming mapping? Then which kind of dma mapping is it? I could miss something important to understand this which is obvious to other people, I will make time to check.
diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c index 9478eccd1c8e..e66847aeac67 100644 --- a/kernel/dma/mapping.c +++ b/kernel/dma/mapping.c @@ -543,10 +543,11 @@ static struct page *__dma_alloc_pages(struct device *dev, size_t size, { const struct dma_map_ops *ops = get_dma_ops(dev); - if (WARN_ON_ONCE(!dev->coherent_dma_mask)) - return NULL; - if (WARN_ON_ONCE(gfp & (__GFP_DMA | __GFP_DMA32 | __GFP_HIGHMEM))) - return NULL; + if (WARN_ON_ONCE(!dev->dma_mask)) + return NULL; + + /* let the implementation decide on the zone to allocate from: */ + gfp &= ~(__GFP_DMA | __GFP_DMA32 | __GFP_HIGHMEM); size = PAGE_ALIGN(size); if (dma_alloc_direct(dev, ops))
For newly added streaming mapping APIs, the internal core function __dma_alloc_pages() should check dev->dma_mask, but not ev->coherent_dma_mask which is for coherent mapping. Meanwhile, just filter out gfp flags if they are any of __GFP_DMA, __GFP_DMA32 and __GFP_HIGHMEM, but not fail it. This change makes it consistent with coherent mapping allocs. Signed-off-by: Baoquan He <bhe@redhat.com> --- kernel/dma/mapping.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)