Re: [PATCH 2/2] dmaengine: idxd: add support for configurable max wq batch size

[Dave Jiang <dave.jiang@xxxxxxxxx>]

On 8/28/2020 3:55 AM, Vinod Koul wrote:
> On 25-08-20, 13:25, Dave Jiang wrote:
> > Add sysfs attribute max_batch_size to wq in order to allow the max batch
> > size configured on a per wq basis. Add support code to configure
> > the valid user input on wq enable. This is a performance tuning
> > parameter.
> >
> > Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> > ---
> >   drivers/dma/idxd/device.c |    2 +-
> >   drivers/dma/idxd/idxd.h   |    1 +
> >   drivers/dma/idxd/init.c   |    1 +
> >   drivers/dma/idxd/sysfs.c  |   38 ++++++++++++++++++++++++++++++++++++++
> >   4 files changed, 41 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
> > index b8dbb7001933..00dab1465ca3 100644
> > --- a/drivers/dma/idxd/device.c
> > +++ b/drivers/dma/idxd/device.c
> > @@ -530,7 +530,7 @@ static int idxd_wq_config_write(struct idxd_wq *wq)
> >   
> >   	/* bytes 12-15 */
> >   	wq->wqcfg.max_xfer_shift = ilog2(wq->max_xfer_bytes);
> > -	wq->wqcfg.max_batch_shift = idxd->hw.gen_cap.max_batch_shift;
> > +	wq->wqcfg.max_batch_shift = ilog2(wq->max_batch_size);
> >   
> >   	dev_dbg(dev, "WQ %d CFGs\n", wq->id);
> >   	for (i = 0; i < 8; i++) {
> > diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
> > index 81db2a472822..e8bec6eb9f7e 100644
> > --- a/drivers/dma/idxd/idxd.h
> > +++ b/drivers/dma/idxd/idxd.h
> > @@ -115,6 +115,7 @@ struct idxd_wq {
> >   	struct dma_chan dma_chan;
> >   	char name[WQ_NAME_SIZE + 1];
> >   	u64 max_xfer_bytes;
> > +	u32 max_batch_size;
> >   };
> >   
> >   struct idxd_engine {
> > diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> > index e5ed5750a6d0..11e5ce168177 100644
> > --- a/drivers/dma/idxd/init.c
> > +++ b/drivers/dma/idxd/init.c
> > @@ -177,6 +177,7 @@ static int idxd_setup_internals(struct idxd_device *idxd)
> >   		mutex_init(&wq->wq_lock);
> >   		wq->idxd_cdev.minor = -1;
> >   		wq->max_xfer_bytes = idxd->max_xfer_bytes;
> > +		wq->max_batch_size = idxd->max_batch_size;
> >   	}
> >   
> >   	for (i = 0; i < idxd->max_engines; i++) {
> > diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
> > index 26b3ace66782..c5f19802cb9e 100644
> > --- a/drivers/dma/idxd/sysfs.c
> > +++ b/drivers/dma/idxd/sysfs.c
> > @@ -1103,6 +1103,43 @@ static struct device_attribute dev_attr_wq_max_transfer_size =
> >   		__ATTR(max_transfer_size, 0644,
> >   		       wq_max_transfer_size_show, wq_max_transfer_size_store);
> >   
> > +static ssize_t wq_max_batch_size_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +	struct idxd_wq *wq = container_of(dev, struct idxd_wq, conf_dev);
> > +
> > +	return sprintf(buf, "%u\n", wq->max_batch_size);
> > +}
> > +
> > +static ssize_t wq_max_batch_size_store(struct device *dev, struct device_attribute *attr,
> > +				       const char *buf, size_t count)
> > +{
> > +	struct idxd_wq *wq = container_of(dev, struct idxd_wq, conf_dev);
> > +	struct idxd_device *idxd = wq->idxd;
> > +	u32 batch_size;
> > +	int rc;
> > +
> > +	if (wq->state != IDXD_WQ_DISABLED)
> > +		return -EPERM;
> > +
> > +	rc = kstrtou32(buf, 10, &batch_size);
> > +	if (rc < 0)
> > +		return -EINVAL;
> > +
> > +	if (batch_size == 0)
> > +		return -EINVAL;
>
> seems quite similar to previous patch, maybe a helper to get the value?

Ok I'll look into that.

> > +
> > +	batch_size = roundup_pow_of_two(batch_size);
> > +	if (batch_size > idxd->max_batch_size)
> > +		return -EINVAL;
> > +
> > +	wq->max_batch_size = batch_size;
> > +
> > +	return count;
> > +}
> > +
> > +static struct device_attribute dev_attr_wq_max_batch_size =
> > +		__ATTR(max_batch_size, 0644, wq_max_batch_size_show, wq_max_batch_size_store);
> > +
> >   static struct attribute *idxd_wq_attributes[] = {
> >   	&dev_attr_wq_clients.attr,
> >   	&dev_attr_wq_state.attr,
> > @@ -1114,6 +1151,7 @@ static struct attribute *idxd_wq_attributes[] = {
> >   	&dev_attr_wq_name.attr,
> >   	&dev_attr_wq_cdev_minor.attr,
> >   	&dev_attr_wq_max_transfer_size.attr,
> > +	&dev_attr_wq_max_batch_size.attr,
>
> ABI?

Yes will add.

> >   	NULL,
> >   };
> >