Re: [PATCH] dmaengine: idxd: add knob for enqcmds retries

[Dave Jiang <dave.jiang@intel.com>]

On 11/21/2021 11:16 PM, Vinod Koul wrote:
> On 03-11-21, 10:23, Dave Jiang wrote:
>> Add a sysfs knob to allow tuning of retries for the kernel ENQCMDS
>> descriptor submission. While on host, it is not as likely that ENQCMDS
>> return busy during normal operations due to the driver controlling the
>> number of descriptors allocated for submission. However, when the driver is
>> operating as a guest driver, the chance of retry goes up significantly due
>> to sharing a wq with multiple VMs. A default value is provided with the
>> system admin being able to tune the value on a per WQ basis.
>>
>> Suggested-by: Sanjay Kumar <sanjay.k.kumar@intel.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   Documentation/ABI/stable/sysfs-driver-dma-idxd |    6 ++++
>>   drivers/dma/idxd/device.c                      |    1 +
>>   drivers/dma/idxd/idxd.h                        |    4 +++
>>   drivers/dma/idxd/init.c                        |    1 +
>>   drivers/dma/idxd/irq.c                         |    2 +
>>   drivers/dma/idxd/submit.c                      |   32 ++++++++++++++++++-----
>>   drivers/dma/idxd/sysfs.c                       |   33 ++++++++++++++++++++++++
>>   7 files changed, 71 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/ABI/stable/sysfs-driver-dma-idxd b/Documentation/ABI/stable/sysfs-driver-dma-idxd
>> index df4afbccf037..fd1a611df510 100644
>> --- a/Documentation/ABI/stable/sysfs-driver-dma-idxd
>> +++ b/Documentation/ABI/stable/sysfs-driver-dma-idxd
>> @@ -220,6 +220,12 @@ Contact:	dmaengine@vger.kernel.org
>>   Description:	Show the current number of entries in this WQ if WQ Occupancy
>>   		Support bit WQ capabilities is 1.
>>   
>> +What:		/sys/bus/dsa/devices/wq<m>.<n>/enqcmds_retries
>> +Date		Oct 29, 2021
>> +KernelVersion:	5.17.0
>> +Contact:	dmaengine@vger.kernel.org
>> +Description:	Indicate the number of retires for an enqcmds submission on a shared wq.
>> +
>>   What:           /sys/bus/dsa/devices/engine<m>.<n>/group_id
>>   Date:           Oct 25, 2019
>>   KernelVersion:  5.6.0
>> diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
>> index 36e213a8108d..5a50ee6f6881 100644
>> --- a/drivers/dma/idxd/device.c
>> +++ b/drivers/dma/idxd/device.c
>> @@ -387,6 +387,7 @@ static void idxd_wq_disable_cleanup(struct idxd_wq *wq)
>>   	wq->threshold = 0;
>>   	wq->priority = 0;
>>   	wq->ats_dis = 0;
>> +	wq->enqcmds_retries = IDXD_ENQCMDS_RETRIES;
>>   	clear_bit(WQ_FLAG_DEDICATED, &wq->flags);
>>   	clear_bit(WQ_FLAG_BLOCK_ON_FAULT, &wq->flags);
>>   	memset(wq->name, 0, WQ_NAME_SIZE);
>> diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
>> index 89e98d69115b..6fe9c7bf78ac 100644
>> --- a/drivers/dma/idxd/idxd.h
>> +++ b/drivers/dma/idxd/idxd.h
>> @@ -52,6 +52,8 @@ enum idxd_type {
>>   #define IDXD_NAME_SIZE		128
>>   #define IDXD_PMU_EVENT_MAX	64
>>   
>> +#define IDXD_ENQCMDS_RETRIES	32
>> +
>>   struct idxd_device_driver {
>>   	const char *name;
>>   	enum idxd_dev_type *type;
>> @@ -173,6 +175,7 @@ struct idxd_dma_chan {
>>   struct idxd_wq {
>>   	void __iomem *portal;
>>   	u32 portal_offset;
>> +	unsigned int enqcmds_retries;
>>   	struct percpu_ref wq_active;
>>   	struct completion wq_dead;
>>   	struct completion wq_resurrect;
>> @@ -584,6 +587,7 @@ int idxd_wq_init_percpu_ref(struct idxd_wq *wq);
>>   int idxd_submit_desc(struct idxd_wq *wq, struct idxd_desc *desc);
>>   struct idxd_desc *idxd_alloc_desc(struct idxd_wq *wq, enum idxd_op_type optype);
>>   void idxd_free_desc(struct idxd_wq *wq, struct idxd_desc *desc);
>> +int idxd_enqcmds(struct idxd_wq *wq, void __iomem *portal, const void *desc);
>>   
>>   /* dmaengine */
>>   int idxd_register_dma_device(struct idxd_device *idxd);
>> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
>> index 94ecd4bf0f0e..8b3afce9ea67 100644
>> --- a/drivers/dma/idxd/init.c
>> +++ b/drivers/dma/idxd/init.c
>> @@ -248,6 +248,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>>   		init_completion(&wq->wq_resurrect);
>>   		wq->max_xfer_bytes = WQ_DEFAULT_MAX_XFER;
>>   		wq->max_batch_size = WQ_DEFAULT_MAX_BATCH;
>> +		wq->enqcmds_retries = IDXD_ENQCMDS_RETRIES;
>>   		wq->wqcfg = kzalloc_node(idxd->wqcfg_size, GFP_KERNEL, dev_to_node(dev));
>>   		if (!wq->wqcfg) {
>>   			put_device(conf_dev);
>> diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c
>> index a3bf3ea84587..0b0055a0ad2a 100644
>> --- a/drivers/dma/idxd/irq.c
>> +++ b/drivers/dma/idxd/irq.c
>> @@ -98,7 +98,7 @@ static void idxd_int_handle_revoke_drain(struct idxd_irq_entry *ie)
>>   	if (wq_dedicated(wq)) {
>>   		iosubmit_cmds512(portal, &desc, 1);
>>   	} else {
>> -		rc = enqcmds(portal, &desc);
>> +		rc = idxd_enqcmds(wq, portal, &desc);
>>   		/* This should not fail unless hardware failed. */
>>   		if (rc < 0)
>>   			dev_warn(dev, "Failed to submit drain desc on wq %d\n", wq->id);
>> diff --git a/drivers/dma/idxd/submit.c b/drivers/dma/idxd/submit.c
>> index 776fa81db61d..dd897accb9fb 100644
>> --- a/drivers/dma/idxd/submit.c
>> +++ b/drivers/dma/idxd/submit.c
>> @@ -123,6 +123,30 @@ static void llist_abort_desc(struct idxd_wq *wq, struct idxd_irq_entry *ie,
>>   		idxd_dma_complete_txd(found, IDXD_COMPLETE_ABORT, false);
>>   }
>>   
>> +
> This blank should be removed
Ooops. Will remove.
>
>> +/*
>> + * ENQCMDS typically fail when the WQ is inactive or busy. On host submission, the driver
>> + * has better control of number of descriptors being submitted to a shared wq by limiting
>> + * the number of driver allocated descriptors to the wq size. However, when the swq is
>> + * exported to a guest kernel, it may be shared with multiple guest kernels. This means
>> + * the likelihood of getting busy returned on the swq when submitting goes significantly up.
>> + * Having a tunable retry mechanism allows the driver to keep trying for a bit before giving
>> + * up. The sysfs knob can be tuned by the system administrator.
>> + */
>> +int idxd_enqcmds(struct idxd_wq *wq, void __iomem *portal, const void *desc)
>> +{
>> +	int rc, retries = 0;
>> +
>> +	do {
>> +		rc = enqcmds(portal, desc);
>> +		if (rc == 0)
>> +			break;
>> +		cpu_relax();
>> +	} while (retries++ < wq->enqcmds_retries);
>> +
>> +	return rc;
>> +}
>> +
>>   int idxd_submit_desc(struct idxd_wq *wq, struct idxd_desc *desc)
>>   {
>>   	struct idxd_device *idxd = wq->idxd;
>> @@ -166,13 +190,7 @@ int idxd_submit_desc(struct idxd_wq *wq, struct idxd_desc *desc)
>>   	if (wq_dedicated(wq)) {
>>   		iosubmit_cmds512(portal, desc->hw, 1);
>>   	} else {
>> -		/*
>> -		 * It's not likely that we would receive queue full rejection
>> -		 * since the descriptor allocation gates at wq size. If we
>> -		 * receive a -EAGAIN, that means something went wrong such as the
>> -		 * device is not accepting descriptor at all.
>> -		 */
>> -		rc = enqcmds(portal, desc->hw);
>> +		rc = idxd_enqcmds(wq, portal, desc->hw);
>>   		if (rc < 0) {
>>   			percpu_ref_put(&wq->wq_active);
>>   			/* abort operation frees the descriptor */
>> diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
>> index 90857e776273..7620cf00dabc 100644
>> --- a/drivers/dma/idxd/sysfs.c
>> +++ b/drivers/dma/idxd/sysfs.c
>> @@ -945,6 +945,38 @@ static ssize_t wq_occupancy_show(struct device *dev, struct device_attribute *at
>>   static struct device_attribute dev_attr_wq_occupancy =
>>   		__ATTR(occupancy, 0444, wq_occupancy_show, NULL);
>>   
>> +static ssize_t wq_enqcmds_retries_show(struct device *dev,
>> +				       struct device_attribute *attr, char *buf)
>> +{
>> +	struct idxd_wq *wq = confdev_to_wq(dev);
>> +
>> +	if (wq_dedicated(wq))
>> +		return -EOPNOTSUPP;
>> +
>> +	return sysfs_emit(buf, "%u\n", wq->enqcmds_retries);
>> +}
>> +
>> +static ssize_t wq_enqcmds_retries_store(struct device *dev, struct device_attribute *attr,
>> +					const char *buf, size_t count)
>> +{
>> +	struct idxd_wq *wq = confdev_to_wq(dev);
>> +	int rc;
>> +	unsigned int retries;
>> +
>> +	if (wq_dedicated(wq))
>> +		return -EOPNOTSUPP;
>> +
>> +	rc = kstrtouint(buf, 10, &retries);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	wq->enqcmds_retries = retries;
> Should there be no upper limit on the retries? Surely we dont want
> userspace to write max and let it keep retrying...
Yes will add reasonable limit.