| Message ID | 1401606077-1739-2-git-send-email-eli.billauer@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On 06/01/2014 01:01 AM, Eli Billauer wrote: > dmam_map_single() and dmam_unmap_single() are the managed counterparts > for the respective dma_* functions. > > Note that dmam_map_single() returns a status value rather than the DMA handle. > The DMA handle is passed to the caller through a pointer in the arguments. > > The reason for this API change is that dmam_map_single() allocates memory, > which may fail even if the DMA mapping is successful. On the other hand, > it seems like there's no platform-independent value for dma_handle that can > be used to indicate an error. > > This leaves dmam_map_single() with no safe way to tell its caller that the > memory allocation has failed, except for returning a status as an int. Trying > to keep close to the non-devres API could also tempt programmers into using > dma_mapping_error() on the dmam_* functions. This would work incorrectly on > platforms, for which dma_mapping_error() ignores its argument, and always > returns success. > I see the value of this interface in unmap case, this type of wrapper can release dma buffers, drivers neglected to release leaving dangling buffers. However, driver writers should give it some thought before switching from conventional dma_map_*() interfaces to these proposed managed dma mapping interfaces. These new interfaces shouldn't be treated as drop in replacements to dma_map_*() interfaces. The reasons are: 1. This interface adds an overhead in allocation memory for devres to compared to other dma_map_* wrappers. Users need to be aware of that. This would be okay in the cases where a driver allocates several buffers at init time and uses them. However, several drivers allocate during run-time and release as soon as it is no longer needed. This overhead is going to be in the performance path. 2. It adds a failure case even when dma buffers are available. i.e if if devres alloc fails, it will return failure even if dma map could have succeeded. This is a new failure case for DMA-API. The reason dma_map_*() routines fail now is because there are no buffers available. Drivers handle this error as a retry case. Drivers using dmam_map_single() will have to handle the failure cases differently. Since the return values are different for dmam_map_*(), that is plus that these interfaces can't be drop in replacements to the dma_map_*() interfaces. 3. Similarly, it adds an overhead in releasing memory for devres to compared to other dma_unmap_* wrappers. Users need to be aware of that. This overhead is going to be in the performance path when drivers unmap buffers during run-time. Adding Joerg Roedel to the thread. -- Shuah
On Tue, Jun 03, 2014 at 03:24:20PM -0600, Shuah Khan wrote: > On 06/01/2014 01:01 AM, Eli Billauer wrote: > I see the value of this interface in unmap case, this type of wrapper > can release dma buffers, drivers neglected to release leaving dangling > buffers. > > However, driver writers should give it some thought before switching > from conventional dma_map_*() interfaces to these proposed managed > dma mapping interfaces. These new interfaces shouldn't be treated as > drop in replacements to dma_map_*() interfaces. > > The reasons are: > > 1. This interface adds an overhead in allocation memory for devres to > compared to other dma_map_* wrappers. Users need to be aware of that. > This would be okay in the cases where a driver allocates several > buffers at init time and uses them. However, several drivers allocate > during run-time and release as soon as it is no longer needed. This > overhead is going to be in the performance path. > > 2. It adds a failure case even when dma buffers are available. i.e if > if devres alloc fails, it will return failure even if dma map could > have succeeded. This is a new failure case for DMA-API. > > The reason dma_map_*() routines fail now is because there are no > buffers available. Drivers handle this error as a retry case. > > Drivers using dmam_map_single() will have to handle the failure > cases differently. > > Since the return values are different for dmam_map_*(), that is > plus that these interfaces can't be drop in replacements to the > dma_map_*() interfaces. > > 3. Similarly, it adds an overhead in releasing memory for devres to > compared to other dma_unmap_* wrappers. Users need to be aware of > that. This overhead is going to be in the performance path when > drivers unmap buffers during run-time. I fully agree with the points Shuah brought up here. I don't think it is a good idea to add this kind of resource management to runtime-allocated (and de-allocated) resources of device drivers. Also DMA handles are not something that could be garbage collected at driver unload time. They are a limited resource that may be used up at some point. And the whole point of a devm-API is that code can be simpler because we don't need to de-allocate everything on the error-path or at unload time, no? Besides that, we already have DMA-API debug to find drivers that do not release all their DMA buffers. Joerg -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Wed, Jun 04, 2014 at 01:39:07AM +0200, Joerg Roedel wrote: > I fully agree with the points Shuah brought up here. I don't think it is > a good idea to add this kind of resource management to runtime-allocated > (and de-allocated) resources of device drivers. > > Also DMA handles are not something that could be garbage collected at > driver unload time. They are a limited resource that may be used up at > some point. And the whole point of a devm-API is that code can be > simpler because we don't need to de-allocate everything on the > error-path or at unload time, no? Hmmm? Don't we have drivers which map dma buffers on device init and release them on exit? For dynamic usages, its usefulness is limited especially given that dynamic tracking of buffers usually would involve tracking of other information too in addition to dma buffer pointer themselves. If alloc on init and free on exit is a very rare usage pattern, I have no objection against not adding devm interface for dma mappings. Thanks.
On Wed, Jun 04, 2014 at 10:04:08AM -0400, Tejun Heo wrote: > Hmmm? Don't we have drivers which map dma buffers on device init and > release them on exit? For dynamic usages, its usefulness is limited > especially given that dynamic tracking of buffers usually would > involve tracking of other information too in addition to dma buffer > pointer themselves. If alloc on init and free on exit is a very rare > usage pattern, I have no objection against not adding devm interface > for dma mappings. Yes, but those drivers usually get DMA buffers at init time with the dma_alloc_* interfaces. The dma_map_* interfaces discussed here belong to the streaming DMA-API, so they are usually used for only one DMA transaction before dma_unmap_* is called on them. A devm interface for the dma_alloc_* family of functions would actually make sense, but not for the dma_map_* functions. Joerg -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Wed, Jun 04, 2014 at 04:12:11PM +0200, Joerg Roedel wrote: > Yes, but those drivers usually get DMA buffers at init time with the > dma_alloc_* interfaces. The dma_map_* interfaces discussed here belong > to the streaming DMA-API, so they are usually used for only one DMA > transaction before dma_unmap_* is called on them. > > A devm interface for the dma_alloc_* family of functions would > actually make sense, but not for the dma_map_* functions. Ah, okay. Fair enough. Thanks.
Hi,
I believe that I need a managed dma_map_single() my own driver, which
doesn't fall in the case of a single use: The driver allocates its
buffers with __get_free_pages() (or the to-be managed version of it).
Then it cuts the allocated memory into smaller buffers (in some cases,
and with certain alignment rules), and then calls dma_map_single() to
do the DMA mapping for each. The buffers are held along the driver's
lifetime, with DMA sync API juggling control back and forth to the
hardware. Note that the DMA is noncoherent.
Once could argue, that since dma_map_noncoherent() calls
__get_free_pages() under the hood, I could request the larger chunk from
dma_map_noncoherent(), and cut it into smaller DMA buffers. My concern
is that the dma_sync_single_*() functions expect a DMA handle, not a
physical address I've made up with my own buffer splitting. I don't see
any problem with this trick on platforms I've worked with, but I'm not
sure if this is the proper way to go. dma_map_single(), on the other
hand, returns a DMA handle.
The DMA pool functions could be interesting, but I understand that they
would supply me with coherent memory only.
Anything I missed?
Thanks,
Eli
On 04/06/14 17:14, Tejun Heo wrote:
> Hello,
>
> On Wed, Jun 04, 2014 at 04:12:11PM +0200, Joerg Roedel wrote:
>
>> Yes, but those drivers usually get DMA buffers at init time with the
>> dma_alloc_* interfaces. The dma_map_* interfaces discussed here belong
>> to the streaming DMA-API, so they are usually used for only one DMA
>> transaction before dma_unmap_* is called on them.
>>
>> A devm interface for the dma_alloc_* family of functions would
>> actually make sense, but not for the dma_map_* functions.
>>
> Ah, okay. Fair enough.
>
> Thanks.
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wed, Jun 04, 2014 at 06:03:36PM +0300, Eli Billauer wrote: > I believe that I need a managed dma_map_single() my own driver, > which doesn't fall in the case of a single use: The driver allocates > its buffers with __get_free_pages() (or the to-be managed version of > it). Then it cuts the allocated memory into smaller buffers (in some > cases, and with certain alignment rules), and then calls > dma_map_single() to do the DMA mapping for each. The buffers are > held along the driver's lifetime, with DMA sync API juggling control > back and forth to the hardware. Note that the DMA is noncoherent. What you are trying to do should work with dma_alloc_noncoherent(). The API allows partial syncs on this memory, so you should be fine. The problem with a devm variant of dma_map_* is that it is too easy to misuse or to use it wrong so that a driver could eat up all available DMA handles on some platforms. Joerg -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Joerg. On 05/06/14 00:25, Joerg Roedel wrote: > > What you are trying to do should work with dma_alloc_noncoherent(). The > API allows partial syncs on this memory, so you should be fine. > Please try to put yourself in my position: I have a driver that I care about, which works fine for a few years. It's based upon dma_map_single(), which seems to be the common way to get non-coherent memory, even for the driver's entire lifespan. I realize that dma_alloc_* was the intended way to do it, but fact is that dma_map_* has become the common choice. Now I need to switch to dma_alloc_noncoherent(), which isn't used by many drivers, it seems. It should work the same, but there's always the worry if I'll cover all the corners. So will I take this risk of a nasty DMA bug on some esoteric platform, just to cut some lines in the code? And if I choose to keep the unmanaged dma_map_single(), maybe I'll mess up if I convert other allocations to the managed API? Hmmm, maybe it's best to forget all about it. > The problem with a devm variant of dma_map_* is that it is too easy to > misuse or to use it wrong so that a driver could eat up all available > DMA handles on some platforms. > I suppose you mean that those who use dma_map_* for a one-off DMA session will drop the unmapping, thinking that it's "managed anyhow"...? Well, you can say that about any of the managed functions. For example, when devm_kzalloc() was introduced, someone maybe argued that people would drop kfree()s where they shouldn't, causing memory leaks. So I think it boils down to whether devres is a good idea or not. Someone who thinks it's bad, will reject any new API, referring to memory efficiency, additional causes of failure and the risk of misleading the herd. But if devres is to become commonly used in the future, I think drop-in replacements are necessary. In my opinion, telling people to adopt another methodology (e.g. dma_alloc_noncoherent vs. mapping), even if functionally equivalent, is a good way to make sure devres is never adopted. Regards, Eli > Joerg > > > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 06, 2014 at 02:45:06PM +0300, Eli Billauer wrote: > Hello Joerg. > > > On 05/06/14 00:25, Joerg Roedel wrote: > > > >What you are trying to do should work with dma_alloc_noncoherent(). The > >API allows partial syncs on this memory, so you should be fine. > Please try to put yourself in my position: I have a driver that I care > about, which works fine for a few years. It's based upon dma_map_single(), > which seems to be the common way to get non-coherent memory, even for the > driver's entire lifespan. I realize that dma_alloc_* was the intended way to > do it, but fact is that dma_map_* has become the common choice. Is your driver in the kernel tree? If not, you really are on your own :( greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/06/14 19:01, Greg KH wrote: >> Please try to put yourself in my position: I have a driver that I care >> > about, which works fine for a few years. It's based upon dma_map_single(), >> > which seems to be the common way to get non-coherent memory, even for the >> > driver's entire lifespan. I realize that dma_alloc_* was the intended way to >> > do it, but fact is that dma_map_* has become the common choice. >> > Is your driver in the kernel tree? If not, you really are on your own :( > It's the Xillybus driver in the staging area. I don't know if this counts for being in the kernel tree... The suggested patchset would allow replacing my use of dma_map_single() with a managed version of that function. This will clean the driver's code considerably. But I think that the discussion here is if it's valid to use dma_map_single() for a device-permanent DMA mapping, or if dma_alloc_noncoherent() is the only way. If the answer is no, there's quite obviously no point in a devres version for that function. Regards, Eli > greg k-h > > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/06/2014 10:21 AM, Eli Billauer wrote: > On 06/06/14 19:01, Greg KH wrote: >>> Please try to put yourself in my position: I have a driver that I care >>> > about, which works fine for a few years. It's based upon >>> dma_map_single(), >>> > which seems to be the common way to get non-coherent memory, even >>> for the >>> > driver's entire lifespan. I realize that dma_alloc_* was the >>> intended way to >>> > do it, but fact is that dma_map_* has become the common choice. >>> >> Is your driver in the kernel tree? If not, you really are on your own :( >> > It's the Xillybus driver in the staging area. I don't know if this > counts for being in the kernel tree... > > The suggested patchset would allow replacing my use of dma_map_single() > with a managed version of that function. This will clean the driver's > code considerably. > > But I think that the discussion here is if it's valid to use > dma_map_single() for a device-permanent DMA mapping, or if > dma_alloc_noncoherent() is the only way. If the answer is no, there's > quite obviously no point in a devres version for that function. > Eli, dma_map_single() and dma_unmap_single() are streaming DMA APIs. These are intended for one DMA transfer and unmapped right after it is done. dma buffers are limited shared resources for streaming that are shared by several drivers. Hence the need for use and release model. Please refer to the Using Streaming DMA mappings in DMA-API-HOWTO.txt "- Streaming DMA mappings which are usually mapped for one DMA transfer, unmapped right after it (unless you use dma_sync_* below) and for which hardware can optimize for sequential accesses. This of "streaming" as "asynchronous" or "outside the coherency domain". Good examples of what to use streaming mappings for are: - Networking buffers transmitted/received by a device. - Filesystem buffers written/read by a SCSI device." If I understand your intended usage correctly, you are looking to allocate and hold the buffers for the lifetime of the driver. For such cases, dma_alloc_*() interfaces are the ones to use. Please also refer to DMA-API.txt as well. Hope this helps. -- Shuah
Hello Shuah, We agree that the streaming API was originally *intended* for short map-unmap DMA sessions, and that dma_alloc_noncoherent() was the *intended* API for those who want to hold the DMA during a device's lifetime. We also agree that on some platforms, DMA mappings are precious, and therefore any driver should unmap a region as soon as it's not needed anymore. But if we stick to the citation you gave, it says "...unmapped right after it (unless you use dma_sync_* below)". So even in the streaming API's definition, there's an understanding, that the "streaming" DMA buffer can be held for more than a single session. And a good sync tool for that is made available. Using cross-reference on Linux' code, I get a strong impression, that dma_alloc_NONcoherent() is pretty much unused (I counted 8 drivers). The streaming API's sync functions are heavily used, on the other hand. So one gets a hunch, that there's a lot of use of the streaming API in the kernel tree for long-term DMA mappings. This wasn't the original intention -- we agree on that. But why is it wrong? Assuming that a driver needs to hold a DMA mapping for a long while, why does it matter if it was done with dma_alloc_noncoherent() or with dma_map_*()? They are equally wasteful, aren't they? Why maintaining two API sets doing the same thing? Or is there a subtle functional difference I'm not aware of? Thanks, Eli On 06/06/14 20:02, Shuah Khan wrote: > > dma_map_single() and dma_unmap_single() are streaming DMA APIs. These > are intended for one DMA transfer and unmapped right after it is done. > > dma buffers are limited shared resources for streaming that are > shared by several drivers. Hence the need for use and release > model. > > Please refer to the Using Streaming DMA mappings in DMA-API-HOWTO.txt > > "- Streaming DMA mappings which are usually mapped for one DMA > transfer, unmapped right after it (unless you use dma_sync_* below) > and for which hardware can optimize for sequential accesses. > > This of "streaming" as "asynchronous" or "outside the coherency > domain". > > Good examples of what to use streaming mappings for are: > > - Networking buffers transmitted/received by a device. > - Filesystem buffers written/read by a SCSI device." > > > If I understand your intended usage correctly, you are looking to > allocate and hold the buffers for the lifetime of the driver. For > such cases, dma_alloc_*() interfaces are the ones to use. > > Please also refer to DMA-API.txt as well. Hope this helps. > > -- Shuah > > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt index e1a2707..13b8be0 100644 --- a/Documentation/driver-model/devres.txt +++ b/Documentation/driver-model/devres.txt @@ -266,6 +266,8 @@ DMA dmam_declare_coherent_memory() dmam_pool_create() dmam_pool_destroy() + dmam_map_single() + dmam_unmap_single() PCI pcim_enable_device() : after success, all PCI ops become managed diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c index 0ce39a3..f76ea0f 100644 --- a/drivers/base/dma-mapping.c +++ b/drivers/base/dma-mapping.c @@ -19,6 +19,7 @@ struct dma_devres { size_t size; void *vaddr; dma_addr_t dma_handle; + enum dma_data_direction direction; }; static void dmam_coherent_release(struct device *dev, void *res) @@ -267,3 +268,88 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, return ret; } EXPORT_SYMBOL(dma_common_mmap); + +static int dmam_map_match(struct device *dev, void *res, void *match_data) +{ + struct dma_devres *this = res, *match = match_data; + + if (this->dma_handle == match->dma_handle) { + WARN_ON(this->size != match->size || + this->direction != match->direction); + return 1; + } + return 0; +} + +static void dmam_map_single_release(struct device *dev, void *res) +{ + struct dma_devres *this = res; + + dma_unmap_single(dev, this->dma_handle, this->size, this->direction); +} + +/** + * dmam_map_single - Managed dma_map_single() + * @dev: Device to map DMA region for + * @ptr: Pointer to region + * @size: Size to map + * @direction: The mapping's direction + * @ret_dma_handle: Pointer to return the value of the DMA handle + * + * Managed dma_map_single(). The region mapped using this + * function will be automatically unmapped on driver detach. + * + * RETURNED VALUE: + * 0 is returned on success + * -EIO if the DMA mapping failed + * -ENOMEM on failure to allocate memory for internal data structure + */ +int dmam_map_single(struct device *dev, void *ptr, size_t size, + enum dma_data_direction direction, + dma_addr_t *ret_dma_handle) + +{ + struct dma_devres *dr; + dma_addr_t dma_handle; + + dr = devres_alloc(dmam_map_single_release, sizeof(*dr), GFP_KERNEL); + if (!dr) + return -ENOMEM; + + dma_handle = dma_map_single(dev, ptr, size, direction); + if (dma_mapping_error(dev, dma_handle)) { + devres_free(dr); + return -EIO; + } + + *ret_dma_handle = dma_handle; + + dr->vaddr = ptr; + dr->dma_handle = dma_handle; + dr->size = size; + dr->direction = direction; + + devres_add(dev, dr); + + return 0; /* Success */ +} +EXPORT_SYMBOL(dmam_map_single); + +/** + * dmam_unmap_single - Managed dma_unmap_single() + * @dev: Device to map DMA region for + * @dma_handle: DMA handle of the region to unmap + * @size: Size to unmap + * @direction: The mapping's direction + * + * Managed dma_unmap_single(). + */ +void dmam_unmap_single(struct device *dev, dma_addr_t dma_handle, + size_t size, enum dma_data_direction direction) +{ + struct dma_devres match_data = { size, NULL, dma_handle, direction }; + + WARN_ON(devres_release(dev, dmam_map_single_release, dmam_map_match, + &match_data)); +} +EXPORT_SYMBOL(dmam_unmap_single); diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index fd4aee2..cad63de 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -233,7 +233,11 @@ static inline void dmam_release_declared_memory(struct device *dev) { } #endif /* ARCH_HAS_DMA_DECLARE_COHERENT_MEMORY */ - +int dmam_map_single(struct device *dev, void *ptr, size_t size, + enum dma_data_direction direction, + dma_addr_t *ret_dma_handle); +void dmam_unmap_single(struct device *dev, dma_addr_t dma_handle, + size_t size, enum dma_data_direction direction); #ifndef CONFIG_HAVE_DMA_ATTRS struct dma_attrs;
dmam_map_single() and dmam_unmap_single() are the managed counterparts for the respective dma_* functions. Note that dmam_map_single() returns a status value rather than the DMA handle. The DMA handle is passed to the caller through a pointer in the arguments. The reason for this API change is that dmam_map_single() allocates memory, which may fail even if the DMA mapping is successful. On the other hand, it seems like there's no platform-independent value for dma_handle that can be used to indicate an error. This leaves dmam_map_single() with no safe way to tell its caller that the memory allocation has failed, except for returning a status as an int. Trying to keep close to the non-devres API could also tempt programmers into using dma_mapping_error() on the dmam_* functions. This would work incorrectly on platforms, for which dma_mapping_error() ignores its argument, and always returns success. Thanks to Tejun Heo for suggesting this API. Signed-off-by: Eli Billauer <eli.billauer@gmail.com> --- Documentation/driver-model/devres.txt | 2 + drivers/base/dma-mapping.c | 86 +++++++++++++++++++++++++++++++++ include/linux/dma-mapping.h | 6 ++- 3 files changed, 93 insertions(+), 1 deletions(-)