| Message ID | 20180501132411.2311-1-michel@daenzer.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 01.05.2018 um 15:24 schrieb Michel Dänzer: > From: Michel Dänzer <michel.daenzer@amd.com> > > The result was printing the warning only when we were explicitly asked > not to. > > Cc: stable@vger.kernel.org > Fixes: 0176adb004065d6815a8e67946752df4cd947c5b "swiotlb: refactor > coherent buffer allocation" > Signed-off-by: Michel Dänzer <michel.daenzer@amd.com> Good catch, looked at the code multiple times and haven't seen that myself :) Reviewed-by: Christian König <christian.koenig@amd.com>. Christian. > --- > lib/swiotlb.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/swiotlb.c b/lib/swiotlb.c > index c43ec2271469..e9ac21540628 100644 > --- a/lib/swiotlb.c > +++ b/lib/swiotlb.c > @@ -750,7 +750,7 @@ swiotlb_alloc_buffer(struct device *dev, size_t size, dma_addr_t *dma_handle, > swiotlb_tbl_unmap_single(dev, phys_addr, size, DMA_TO_DEVICE, > DMA_ATTR_SKIP_CPU_SYNC); > out_warn: > - if ((attrs & DMA_ATTR_NO_WARN) && printk_ratelimit()) { > + if (!(attrs & DMA_ATTR_NO_WARN) && printk_ratelimit()) { > dev_warn(dev, > "swiotlb: coherent allocation failed, size=%zu\n", > size);
On Wed, May 2, 2018 at 11:49 AM, Christian König <ckoenig.leichtzumerken@gmail.com> wrote: > Am 01.05.2018 um 15:24 schrieb Michel Dänzer: >> >> From: Michel Dänzer <michel.daenzer@amd.com> >> >> The result was printing the warning only when we were explicitly asked >> not to. >> >> Cc: stable@vger.kernel.org >> Fixes: 0176adb004065d6815a8e67946752df4cd947c5b "swiotlb: refactor >> coherent buffer allocation" >> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com> > > > Good catch, looked at the code multiple times and haven't seen that myself > :) > > Reviewed-by: Christian König <christian.koenig@amd.com>. Other dma-api backends like cma just shut up when __GFP_NOWARN is passed. And afaiui Christoph Hellwig has plans to nuke the DMA_ATTR stuff (or at least clean it up) - should we just remove DMA_ATTR_NO_WARN and instead only look at __GFP_NOWARN? For context the CMA patch: commit ef4650144e76ae361fe4b8c9a0afcd53074cd520 Author: Boris Brezillon <boris.brezillon@free-electrons.com> Date: Fri Oct 13 15:58:01 2017 -0700 mm/cma.c: take __GFP_NOWARN into account in cma_alloc() Or maybe we should at least enforce that both or none are set, for consistency for now? Cheers, Daniel > > Christian. > >> --- >> lib/swiotlb.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/lib/swiotlb.c b/lib/swiotlb.c >> index c43ec2271469..e9ac21540628 100644 >> --- a/lib/swiotlb.c >> +++ b/lib/swiotlb.c >> @@ -750,7 +750,7 @@ swiotlb_alloc_buffer(struct device *dev, size_t size, >> dma_addr_t *dma_handle, >> swiotlb_tbl_unmap_single(dev, phys_addr, size, DMA_TO_DEVICE, >> DMA_ATTR_SKIP_CPU_SYNC); >> out_warn: >> - if ((attrs & DMA_ATTR_NO_WARN) && printk_ratelimit()) { >> + if (!(attrs & DMA_ATTR_NO_WARN) && printk_ratelimit()) { >> dev_warn(dev, >> "swiotlb: coherent allocation failed, size=%zu\n", >> size); > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, May 02, 2018 at 02:18:56PM +0200, Daniel Vetter wrote: > Other dma-api backends like cma just shut up when __GFP_NOWARN is > passed. And afaiui Christoph Hellwig has plans to nuke the DMA_ATTR > stuff (or at least clean it up) - should we just remove > DMA_ATTR_NO_WARN and instead only look at __GFP_NOWARN? No. __GFP_NOWARN (and gfp_t flags in general) are the wrong interface for dma allocations and just cause problems. I actually plan to get rid of the gfp_t argument in dma_alloc_attrs sooner, and only allow either GFP_KERNEL or GFP_DMA passed in dma_alloc_coherent. > Or maybe we should at least enforce that both or none are set, for > consistency for now? The interface should be DMA_ATTR_NO_WARN. __GFP_NOWARN in this context was never documented, and just slipped in.
On Tue, May 01, 2018 at 03:24:11PM +0200, Michel Dänzer wrote: > From: Michel Dänzer <michel.daenzer@amd.com> > > The result was printing the warning only when we were explicitly asked > not to. Thanks, applied to the dma-mapping tree for 4.17.
On 2018-05-02 02:41 PM, Christoph Hellwig wrote: > On Wed, May 02, 2018 at 02:18:56PM +0200, Daniel Vetter wrote: >> Other dma-api backends like cma just shut up when __GFP_NOWARN is >> passed. And afaiui Christoph Hellwig has plans to nuke the DMA_ATTR >> stuff (or at least clean it up) - should we just remove >> DMA_ATTR_NO_WARN and instead only look at __GFP_NOWARN? > > No. __GFP_NOWARN (and gfp_t flags in general) are the wrong interface > for dma allocations and just cause problems. I actually plan to > get rid of the gfp_t argument in dma_alloc_attrs sooner, and only > allow either GFP_KERNEL or GFP_DMA passed in dma_alloc_coherent. How about GFP_TRANSHUGE_LIGHT? TTM uses that to opportunistically allocate huge pages (GFP_TRANSHUGE can result in unacceptably long delays with memory pressure).
On Wed, May 02, 2018 at 04:31:09PM +0200, Michel Dänzer wrote: > > No. __GFP_NOWARN (and gfp_t flags in general) are the wrong interface > > for dma allocations and just cause problems. I actually plan to > > get rid of the gfp_t argument in dma_alloc_attrs sooner, and only > > allow either GFP_KERNEL or GFP_DMA passed in dma_alloc_coherent. > > How about GFP_TRANSHUGE_LIGHT? TTM uses that to opportunistically > allocate huge pages (GFP_TRANSHUGE can result in unacceptably long > delays with memory pressure). Well, that is exactly what I don't want drivers to do - same for __GFP_COMP in some drm code. This very much assumes the page allocator is used to back dma allocations, which very often it actually isn't, and any use of magic gfp flags creates a tight coupling of consumers with a specific implementation. In general I can't think of a good reason not to actually use GFP_TRANSHUGE_LIGHT by default in the dma allocator unless DMA_ATTR_ALLOC_SINGLE_PAGES is set. Can you prepare a patch for that?
On 2018-05-02 06:21 PM, Christoph Hellwig wrote: > On Wed, May 02, 2018 at 04:31:09PM +0200, Michel Dänzer wrote: >>> No. __GFP_NOWARN (and gfp_t flags in general) are the wrong interface >>> for dma allocations and just cause problems. I actually plan to >>> get rid of the gfp_t argument in dma_alloc_attrs sooner, and only >>> allow either GFP_KERNEL or GFP_DMA passed in dma_alloc_coherent. >> >> How about GFP_TRANSHUGE_LIGHT? TTM uses that to opportunistically >> allocate huge pages (GFP_TRANSHUGE can result in unacceptably long >> delays with memory pressure). > > Well, that is exactly what I don't want drivers to do - same for > __GFP_COMP in some drm code. This very much assumes the page allocator > is used to back dma allocations, which very often it actually isn't, and > any use of magic gfp flags creates a tight coupling of consumers with a > specific implementation. > > In general I can't think of a good reason not to actually use > GFP_TRANSHUGE_LIGHT by default in the dma allocator unless > DMA_ATTR_ALLOC_SINGLE_PAGES is set. Can you prepare a patch for that? I'm afraid I'll have to leave that to somebody else.
Am 02.05.2018 um 18:59 schrieb Michel Dänzer: > On 2018-05-02 06:21 PM, Christoph Hellwig wrote: >> On Wed, May 02, 2018 at 04:31:09PM +0200, Michel Dänzer wrote: >>>> No. __GFP_NOWARN (and gfp_t flags in general) are the wrong interface >>>> for dma allocations and just cause problems. I actually plan to >>>> get rid of the gfp_t argument in dma_alloc_attrs sooner, and only >>>> allow either GFP_KERNEL or GFP_DMA passed in dma_alloc_coherent. >>> How about GFP_TRANSHUGE_LIGHT? TTM uses that to opportunistically >>> allocate huge pages (GFP_TRANSHUGE can result in unacceptably long >>> delays with memory pressure). >> Well, that is exactly what I don't want drivers to do - same for >> __GFP_COMP in some drm code. This very much assumes the page allocator >> is used to back dma allocations, which very often it actually isn't, and >> any use of magic gfp flags creates a tight coupling of consumers with a >> specific implementation. >> >> In general I can't think of a good reason not to actually use >> GFP_TRANSHUGE_LIGHT by default in the dma allocator unless >> DMA_ATTR_ALLOC_SINGLE_PAGES is set. Can you prepare a patch for that? > I'm afraid I'll have to leave that to somebody else. Coming back to this topic once more, sorry for the delay but busy as usual :) What exactly do you mean with "dma allocator" here? The TTM allocator using the dma_alloc_coherent calls? Or the swiotlb implementation of the calls? Christian.
On Tue, May 22, 2018 at 03:13:58PM +0200, Christian König wrote: > Am 02.05.2018 um 18:59 schrieb Michel Dänzer: >> On 2018-05-02 06:21 PM, Christoph Hellwig wrote: >>> On Wed, May 02, 2018 at 04:31:09PM +0200, Michel Dänzer wrote: >>>>> No. __GFP_NOWARN (and gfp_t flags in general) are the wrong interface >>>>> for dma allocations and just cause problems. I actually plan to >>>>> get rid of the gfp_t argument in dma_alloc_attrs sooner, and only >>>>> allow either GFP_KERNEL or GFP_DMA passed in dma_alloc_coherent. >>>> How about GFP_TRANSHUGE_LIGHT? TTM uses that to opportunistically >>>> allocate huge pages (GFP_TRANSHUGE can result in unacceptably long >>>> delays with memory pressure). >>> Well, that is exactly what I don't want drivers to do - same for >>> __GFP_COMP in some drm code. This very much assumes the page allocator >>> is used to back dma allocations, which very often it actually isn't, and >>> any use of magic gfp flags creates a tight coupling of consumers with a >>> specific implementation. >>> >>> In general I can't think of a good reason not to actually use >>> GFP_TRANSHUGE_LIGHT by default in the dma allocator unless >>> DMA_ATTR_ALLOC_SINGLE_PAGES is set. Can you prepare a patch for that? >> I'm afraid I'll have to leave that to somebody else. > > Coming back to this topic once more, sorry for the delay but busy as usual > :) > > What exactly do you mean with "dma allocator" here? The TTM allocator using > the dma_alloc_coherent calls? Or the swiotlb implementation of the calls? dma allocatr in this case: backends for dma_alloc_coherent/ dma_alloc_attrs. Most importantly dma_direct_alloc. But while we're at it I can't actually see any GFP_TRANSHUGE_LIGHT usage in TTM, just plain old GFP_TRANSHUGE.
On 2018-05-25 10:41 AM, Christoph Hellwig wrote: > On Tue, May 22, 2018 at 03:13:58PM +0200, Christian König wrote: >> Am 02.05.2018 um 18:59 schrieb Michel Dänzer: >>> On 2018-05-02 06:21 PM, Christoph Hellwig wrote: >>>> On Wed, May 02, 2018 at 04:31:09PM +0200, Michel Dänzer wrote: >>>>>> No. __GFP_NOWARN (and gfp_t flags in general) are the wrong interface >>>>>> for dma allocations and just cause problems. I actually plan to >>>>>> get rid of the gfp_t argument in dma_alloc_attrs sooner, and only >>>>>> allow either GFP_KERNEL or GFP_DMA passed in dma_alloc_coherent. >>>>> How about GFP_TRANSHUGE_LIGHT? TTM uses that to opportunistically >>>>> allocate huge pages (GFP_TRANSHUGE can result in unacceptably long >>>>> delays with memory pressure). >>>> Well, that is exactly what I don't want drivers to do - same for >>>> __GFP_COMP in some drm code. This very much assumes the page allocator >>>> is used to back dma allocations, which very often it actually isn't, and >>>> any use of magic gfp flags creates a tight coupling of consumers with a >>>> specific implementation. >>>> >>>> In general I can't think of a good reason not to actually use >>>> GFP_TRANSHUGE_LIGHT by default in the dma allocator unless >>>> DMA_ATTR_ALLOC_SINGLE_PAGES is set. Can you prepare a patch for that? >>> I'm afraid I'll have to leave that to somebody else. >> >> Coming back to this topic once more, sorry for the delay but busy as usual >> :) >> >> What exactly do you mean with "dma allocator" here? The TTM allocator using >> the dma_alloc_coherent calls? Or the swiotlb implementation of the calls? > > dma allocatr in this case: backends for dma_alloc_coherent/ > dma_alloc_attrs. Most importantly dma_direct_alloc. > > But while we're at it I can't actually see any GFP_TRANSHUGE_LIGHT > usage in TTM, just plain old GFP_TRANSHUGE. See https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=da291320baec914f0bb4e65a9dccb86bd6c728f2 .
diff --git a/lib/swiotlb.c b/lib/swiotlb.c index c43ec2271469..e9ac21540628 100644 --- a/lib/swiotlb.c +++ b/lib/swiotlb.c @@ -750,7 +750,7 @@ swiotlb_alloc_buffer(struct device *dev, size_t size, dma_addr_t *dma_handle, swiotlb_tbl_unmap_single(dev, phys_addr, size, DMA_TO_DEVICE, DMA_ATTR_SKIP_CPU_SYNC); out_warn: - if ((attrs & DMA_ATTR_NO_WARN) && printk_ratelimit()) { + if (!(attrs & DMA_ATTR_NO_WARN) && printk_ratelimit()) { dev_warn(dev, "swiotlb: coherent allocation failed, size=%zu\n", size);