On Thu, Aug 8, 2013 at 3:59 AM, Wang, Rui Y <rui.y.w...@intel.com> wrote:
> (resend adding cc list)
The e-mail you are responding to is over a year old, but doesn't
appear to have been accepted. I suppose late is better than never...
Adding Dan Williams new e-mail address and Dave Jiang.
Thanks,
Jon
>
> Hi Jiang,
> See my comments inline.
>
>> -----Original Message-----
>> From: Jiang Liu <liu...@gmail.com>
>> Date: Mon, 14 May 2012 21:47:05 +0800
>> Subject: [RFC PATCH v2 3/7] dmaengine: enhance dmaengine to support DMA
>> device hotplug
>> To: Dan Williams <dan.j.willi...@intel.com>, Maciej Sosnowski
>> <maciej.sosnow...@intel.com>, Vinod Koul <vinod.k...@intel.com>
>> Cc: Jiang Liu <liu...@gmail.com>, Keping Chen <chenkep...@huawei.com>,
>> linux-kernel@vger.kernel.org, linux-...@vger.kernel.org
>>
>> From: Jiang Liu <liu...@gmail.com>
>>
>> From: Jiang Liu <liu...@gmail.com>
>>
>> To support DMA device hotplug, dmaengine must correctly manage reference
>> count for DMA channels. On the other hand, DMA hot path is performance
>> critical, reference count management code should avoid polluting global
>> shared
>> cachelines, otherwise it may cause heavy performance penalty.
>>
>> This patch introduces a lightweight DMA channel reference count management
>> mechanism by using percpu counter. When DMA device hotplug is disabled,
>> there's no performance penalty. When hotplug is enabled, a
>> dma_find_channel()/dma_put_channel() pair adds two local memory accesses
>> to the hot path.
>>
>> Signed-off-by: Jiang Liu <liu...@gmail.com>
>> ---
>> drivers/dma/dmaengine.c | 112
>> +++++++++++++++++++++++++++++++++++++++++----
>> include/linux/dmaengine.h | 29 +++++++++++-
>> 2 files changed, 129 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c index
>> d3b1c48..eca45c0 100644
>> --- a/drivers/dma/dmaengine.c
>> +++ b/drivers/dma/dmaengine.c
>> @@ -61,12 +61,20 @@
>> #include <linux/rculist.h>
>> #include <linux/idr.h>
>> #include <linux/slab.h>
>> +#include <linux/sched.h>
>>
>> static DEFINE_MUTEX(dma_list_mutex);
>> static DEFINE_IDR(dma_idr);
>> static LIST_HEAD(dma_device_list);
>> static long dmaengine_client_count;
>>
>> +#ifdef CONFIG_DMA_ENGINE_HOTPLUG
>> +static atomic_t dmaengine_dirty;
>> +static struct static_key dmaengine_quiesce = STATIC_KEY_INIT_FALSE;
>> +static DECLARE_WAIT_QUEUE_HEAD(dmaengine_wait_queue);
>> +DEFINE_PER_CPU(long, dmaengine_chan_ref_count);
>> +#endif /* CONFIG_DMA_ENGINE_HOTPLUG */
>> +
>> /* --- sysfs implementation --- */
>>
>> /**
>> @@ -305,10 +313,40 @@ arch_initcall(dma_channel_table_init);
>> */
>> struct dma_chan *dma_find_channel(enum dma_transaction_type tx_type)
>> {
>> - return this_cpu_read(channel_table[tx_type]->chan);
>> + struct dma_chan *chan = this_cpu_read(channel_table[tx_type]->chan);
>> +
>> +#ifdef CONFIG_DMA_ENGINE_HOTPLUG
>> + this_cpu_inc(dmaengine_chan_ref_count);
>> + if (static_key_false(&dmaengine_quiesce))
>> + chan = NULL;
>> +#endif
>> +
>> + return chan;
>> }
>> EXPORT_SYMBOL(dma_find_channel);
>>
>> +#ifdef CONFIG_DMA_ENGINE_HOTPLUG
>> +struct dma_chan *dma_get_channel(struct dma_chan *chan) {
>> + if (static_key_false(&dmaengine_quiesce))
>> + atomic_inc(&dmaengine_dirty);
>> + this_cpu_inc(dmaengine_chan_ref_count);
>> +
>> + return chan;
>> +}
>> +EXPORT_SYMBOL(dma_get_channel);
>> +#endif
>> +
>> +/**
>> + * dma_has_capability - check whether any channel supports tx_type
>> + * @tx_type: transaction type
>> + */
>> +bool dma_has_capability(enum dma_transaction_type tx_type) {
>> + return !!this_cpu_read(channel_table[tx_type]->chan);
>> +}
>> +EXPORT_SYMBOL(dma_has_capability);
>> +
>> /*
>> * net_dma_find_channel - find a channel for net_dma
>> * net_dma has alignment requirements
>> @@ -316,10 +354,15 @@ EXPORT_SYMBOL(dma_find_channel); struct
>> dma_chan *net_dma_find_channel(void) {
>> struct dma_chan *chan = dma_find_channel(DMA_MEMCPY);
>> - if (chan && !is_dma_copy_aligned(chan->device, 1, 1, 1))
>> - return NULL;
>>
>> - return chan;
>> + if (chan && is_dma_copy_aligned(chan->device, 1, 1, 1))
>> + return chan;
>> +
>> +#ifdef CONFIG_DMA_ENGINE_HOTPLUG
>> + this_cpu_dec(dmaengine_chan_ref_count);
>> +#endif
>> +
>> + return NULL;
>> }
>> EXPORT_SYMBOL(net_dma_find_channel);
>>
>> @@ -393,6 +436,37 @@ static struct dma_chan *nth_chan(enum
>> dma_transaction_type cap, int n)
>> return ret;
>> }
>>
>> +static void dma_channel_quiesce(void)
>> +{
>> +#ifdef CONFIG_DMA_ENGINE_HOTPLUG
>> + int cpu;
>> + long ref_count;
>> + int quiesce_count = 0;
>> +
>> + static_key_slow_inc(&dmaengine_quiesce);
>> +
>> + do {
>> + atomic_set(&dmaengine_dirty, 0);
>> + schedule_timeout(1);
>> +
>> + if (atomic_read(&dmaengine_dirty)) {
>> + quiesce_count = 0;
>> + continue;
>> + }
>> +
>> + ref_count = 0;
>> + for_each_possible_cpu(cpu)
>> + ref_count += per_cpu(dmaengine_chan_ref_count, cpu);
>> + if (ref_count == 0)
>> + quiesce_count++;
>> + else
>> + quiesce_count = 0;
>> + } while (quiesce_count < 10);
>> +
>> + static_key_slow_dec(&dmaengine_quiesce);
>> +#endif
>> +}
>> +
>
> The purpose of dma_channel_quiesce() is unclear to me. It waits until
> dmaengine_chan_ref_count is 0, but how do you prevent someone from using dma
> after it returns?
>
> <snip>
>
>> @@ -767,14 +848,22 @@ void dma_async_device_unregister(struct
>> dma_device *device)
>>
>> mutex_lock(&dma_list_mutex);
>> list_del_rcu(&device->global_node);
>> - dma_channel_rebalance();
>> + dma_channel_rebalance(true);
>> mutex_unlock(&dma_list_mutex);
>>
>> /* Check whether it's called from module exit function. */
>> if (try_module_get(device->dev->driver->owner)) {
>> +#ifndef CONFIG_DMA_ENGINE_HOTPLUG
>> dev_warn(device->dev,
>> "%s isn't called from module exit function.\n",
>> __func__);
>> +#else
>> + list_for_each_entry(chan, &device->channels, device_node) {
>> + /* TODO: notify clients to release channels*/
>> + wait_event(dmaengine_wait_queue,
>> + chan->client_count == 0);
>> + }
>
> How do you notify clients to release the channels? Is there an updated
> version which handles this? There're resources (e.g. timers) that must be
> free'd, which can only happen when someone calls dma_chan_put() until
> client_count reaches 0 and device_free_chan_resources() gets called.
>
> Rui Wang
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
