[Accel-config] Re: [PATCH v1 1/2] accel-config: Remove cdev related code

[Accel-config] Re: [PATCH v1 1/2] accel-config: Remove cdev related code

[Thomas, Ramesh]

[-- Attachment #1: Type: text/plain, Size: 596 bytes --]

Should we add "cdev path : /dev/dsa/..." to the wq listing?

On Mon, May 17, 2021 at 03:07:23PM, Ramesh Thomas wrote:
> On Mon, May 17, 2021 at 02:58:37PM, Tony Luck wrote:
> > > cdev major/minor method of opening idxd char dev will be deprecated.
> > > Currently even in non uacce mode the char dev is opened using the
> > > device
> > > name at /dev/dsa that is returned by accfg_wq_get_user_dev_path()
> > 
> > Is there an option to accel-config that will show this device path?
> 
> No, it is only returned in the library API. The cdev major/minor was
> being shown in wq listing.

[Accel-config] Re: [PATCH v1 1/2] accel-config: Remove cdev related code

[Dave Jiang]

[-- Attachment #1: Type: text/plain, Size: 1205 bytes --]


On 5/18/2021 11:31 AM, Thomas, Ramesh wrote:
> On Tue, May 18, 2021 at 08:07:04AM -0700, Dave Jiang
> <dave.jiang(a)intel.com> wrote:
>> On 5/17/2021 2:50 PM, ramesh.thomas(a)intel.com wrote:
>>> From: Ramesh Thomas <ramesh.thomas(a)intel.com>
>>>
>>> cdev major/minor method of opening idxd char dev will be deprecated.
>>> Currently even in non uacce mode the char dev is opened using the device
>>> name at /dev/dsa that is returned by accfg_wq_get_user_dev_path()
>> I'm concerned about backward compatibility for apps (Intel user libs)
>> that may still use minor/major API. Do we need to provide some sort of
>> backward compatibility (maybe with warnings?) until they get off this
>> train? Especially they have been doing releases already.
> I think we can leave the API for now as long as driver has backward
> compatibility. Driver was returning error (-6) when cdev_minor was
> accessed. The library was trying to validate cdev_minor after enabling a
> wq and setting cdev_minor = -1 after wq was disabled. Do you think we
> can remove that code?

I think so. Basically lets leave the API call and provide the info if 
available. But no need to do checking.


> -Ramesh

[Accel-config] Re: [PATCH v1 1/2] accel-config: Remove cdev related code

[Thomas, Ramesh]

[-- Attachment #1: Type: text/plain, Size: 1063 bytes --]

On Mon, May 17, 2021 at 04:58:47PM, Tony Luck wrote:
> > Should we add "cdev path : /dev/dsa/..." to the wq listing?
> 
> That's the direction I was thinking (based on the fact that you took
> away cdev major/minor
> and didn't replace them).
> 
> But it isn't all that equivalent.  A command line only solution might
> have had a tough time
> getting from the major/minor devices to actually opening a device.
> 
> So the answer is maybe ... but only if we can convince ourselves that
> somebody is
> going to use "accel-config list" to feed into some script that extracts
> the pathname
> and hands it onto a process to open/mmap.  That seems something of a
> stretch.

accfg_wq_get_user_dev_path() was the replacement. I thought the display
of major/minor in the listing was only for informative purpose i.e. user
can use that info in their application. Note that there is also a
library API to retrieve the major/minor which I think we can leave for
now if users are still using it and driver provides backward
compatibility.

-Ramesh

[Accel-config] Re: [PATCH v1 1/2] accel-config: Remove cdev related code

[Thomas, Ramesh]

[-- Attachment #1: Type: text/plain, Size: 1029 bytes --]

On Tue, May 18, 2021 at 08:07:04AM -0700, Dave Jiang
<dave.jiang(a)intel.com> wrote:
> On 5/17/2021 2:50 PM, ramesh.thomas(a)intel.com wrote:
> > From: Ramesh Thomas <ramesh.thomas(a)intel.com>
> >
> > cdev major/minor method of opening idxd char dev will be deprecated.
> > Currently even in non uacce mode the char dev is opened using the device
> > name at /dev/dsa that is returned by accfg_wq_get_user_dev_path()
> 
> I'm concerned about backward compatibility for apps (Intel user libs) 
> that may still use minor/major API. Do we need to provide some sort of 
> backward compatibility (maybe with warnings?) until they get off this 
> train? Especially they have been doing releases already.

I think we can leave the API for now as long as driver has backward
compatibility. Driver was returning error (-6) when cdev_minor was
accessed. The library was trying to validate cdev_minor after enabling a
wq and setting cdev_minor = -1 after wq was disabled. Do you think we
can remove that code?

-Ramesh

[Accel-config] Re: [PATCH v1 1/2] accel-config: Remove cdev related code

[Dave Jiang]

[-- Attachment #1: Type: text/plain, Size: 8403 bytes --]


On 5/17/2021 2:50 PM, ramesh.thomas(a)intel.com wrote:
> From: Ramesh Thomas <ramesh.thomas(a)intel.com>
>
> cdev major/minor method of opening idxd char dev will be deprecated.
> Currently even in non uacce mode the char dev is opened using the device
> name at /dev/dsa that is returned by accfg_wq_get_user_dev_path()

I'm concerned about backward compatibility for apps (Intel user libs) 
that may still use minor/major API. Do we need to provide some sort of 
backward compatibility (maybe with warnings?) until they get off this 
train? Especially they have been doing releases already.


>
> Signed-off-by: Ramesh Thomas <ramesh.thomas(a)intel.com>
> ---
>   accfg/lib/libaccel-config.sym |  2 -
>   accfg/lib/libaccfg.c          | 80 -----------------------------------
>   accfg/lib/private.h           |  2 -
>   accfg/libaccel_config.h       |  2 -
>   util/json.c                   | 11 -----
>   5 files changed, 97 deletions(-)
>
> diff --git a/accfg/lib/libaccel-config.sym b/accfg/lib/libaccel-config.sym
> index aecc98b..3e86274 100644
> --- a/accfg/lib/libaccel-config.sym
> +++ b/accfg/lib/libaccel-config.sym
> @@ -28,7 +28,6 @@ LIBACCFG_1 {
>   	accfg_device_get_state;
>   	accfg_device_get_max_tokens;
>   	accfg_device_get_max_batch_size;
> -	accfg_device_get_cdev_major;
>   	accfg_device_is_active;
>   	accfg_device_get_token_limit;
>   	accfg_device_set_token_limit;
> @@ -69,7 +68,6 @@ LIBACCFG_1 {
>   	accfg_wq_get_size;
>   	accfg_wq_get_block_on_fault;
>   	accfg_wq_get_state;
> -	accfg_wq_get_cdev_minor;
>   	accfg_wq_get_type;
>   	accfg_wq_get_threshold;
>   	accfg_wq_set_size;
> diff --git a/accfg/lib/libaccfg.c b/accfg/lib/libaccfg.c
> index 0796f08..f0f6bb2 100644
> --- a/accfg/lib/libaccfg.c
> +++ b/accfg/lib/libaccfg.c
> @@ -581,7 +581,6 @@ static void *add_device(void *parent, int id, const char *ctl_base,
>   			"pasid_enabled");
>   	device->max_tokens = accfg_get_param_long(ctx, dfd, "max_tokens");
>   	device->token_limit = accfg_get_param_long(ctx, dfd, "token_limit");
> -	device->cdev_major = accfg_get_param_long(ctx, dfd, "cdev_major");
>   	device->version = accfg_get_param_unsigned_llong(ctx, dfd, "version");
>   	device->device_path = realpath(ctl_base, NULL);
>   	close(dfd);
> @@ -727,7 +726,6 @@ static void *add_wq(void *parent, int id, const char *wq_base,
>   			"block_on_fault");
>   	wq->mode = accfg_get_param_str(ctx, dfd, "mode");
>   	wq->state = accfg_get_param_str(ctx, dfd, "state");
> -	wq->cdev_minor = accfg_get_param_long(ctx, dfd, "cdev_minor");
>   	wq_type = accfg_get_param_str(ctx, dfd, "type");
>   	wq->name = accfg_get_param_str(ctx, dfd, "name");
>   	wq->threshold =  accfg_get_param_long(ctx, dfd, "threshold");
> @@ -1378,12 +1376,6 @@ ACCFG_EXPORT unsigned int accfg_device_get_token_limit(
>   	return device->token_limit;
>   }
>   
> -ACCFG_EXPORT unsigned int accfg_device_get_cdev_major(
> -		struct accfg_device *device)
> -{
> -	return device->cdev_major;
> -}
> -
>   ACCFG_EXPORT unsigned int accfg_device_get_version(
>   		struct accfg_device *device)
>   {
> @@ -1859,11 +1851,6 @@ ACCFG_EXPORT const char *accfg_wq_get_devname(struct accfg_wq *wq)
>   	return devpath_to_devname(wq->wq_path);
>   }
>   
> -ACCFG_EXPORT int accfg_wq_get_cdev_minor(struct accfg_wq *wq)
> -{
> -	return wq->cdev_minor;
> -}
> -
>   ACCFG_EXPORT enum accfg_wq_type accfg_wq_get_type(struct accfg_wq *wq)
>   {
>   	return wq->type;
> @@ -1923,68 +1910,6 @@ ACCFG_EXPORT int accfg_wq_priority_boundary(struct accfg_wq *wq)
>   	return 0;
>   }
>   
> -static int accfg_wq_retrieve_cdev_minor(struct accfg_wq *wq)
> -{
> -	struct accfg_ctx *ctx = accfg_wq_get_ctx(wq);
> -	char *path = wq->wq_buf;
> -	char buf[SYSFS_ATTR_SIZE];
> -	int rc;
> -
> -	rc = sprintf(wq->wq_buf, "%s/%s", wq->wq_path, "cdev_minor");
> -	if (rc < 0)
> -		return -errno;
> -
> -	if (sysfs_read_attr(ctx, path, buf) < 0) {
> -		err(ctx, "%s: retrieve cdev minor failed: '%s': %s\n",
> -				__func__, wq->wq_path, strerror(errno));
> -		return -errno;
> -	}
> -
> -	wq->cdev_minor = atoi(buf);
> -	return 0;
> -}
> -
> -static int accfg_wq_post_enable(struct accfg_wq *wq)
> -{
> -	enum accfg_wq_type type;
> -	int rc;
> -
> -	type = accfg_wq_get_type(wq);
> -
> -	if (type == ACCFG_WQT_USER) {
> -		rc = accfg_wq_retrieve_cdev_minor(wq);
> -		if (rc < 0)
> -			return rc;
> -	}
> -
> -	return 0;
> -}
> -
> -static int accfg_wq_post_disable(struct accfg_wq *wq)
> -{
> -	enum accfg_wq_type type;
> -
> -	type = accfg_wq_get_type(wq);
> -
> -	if (type == ACCFG_WQT_USER)
> -		wq->cdev_minor = -1;
> -
> -	return 0;
> -}
> -
> -static int accfg_wq_control_post_processing(struct accfg_wq *wq,
> -		enum accfg_control_flag flag)
> -{
> -	if (flag == ACCFG_WQ_ENABLE)
> -		return accfg_wq_post_enable(wq);
> -	else if (flag == ACCFG_WQ_DISABLE)
> -		return accfg_wq_post_disable(wq);
> -	else
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
>   static bool accfg_wq_state_expected(struct accfg_wq *wq,
>   		enum accfg_control_flag flag)
>   {
> @@ -2052,11 +1977,6 @@ static int accfg_wq_control(struct accfg_wq *wq, enum accfg_control_flag flag,
>   		return -ENXIO;
>   	}
>   
> -	/* post processing */
> -	rc = accfg_wq_control_post_processing(wq, flag);
> -	if (rc < 0)
> -		return rc;
> -
>   	return 0;
>   }
>   
> diff --git a/accfg/lib/private.h b/accfg/lib/private.h
> index 7f3162e..dc12388 100644
> --- a/accfg/lib/private.h
> +++ b/accfg/lib/private.h
> @@ -50,7 +50,6 @@ struct accfg_device {
>   	int configurable;
>   	int max_tokens;
>   	unsigned int token_limit;
> -	unsigned int cdev_major;
>   	unsigned int version;
>   	uint64_t max_transfer_size;
>   	uint64_t opcap;
> @@ -118,7 +117,6 @@ struct accfg_wq {
>   	int priv;
>   	int priority;
>   	int block_on_fault;
> -	int cdev_minor;
>   	unsigned int threshold;
>   	char *mode;
>   	char *name;
> diff --git a/accfg/libaccel_config.h b/accfg/libaccel_config.h
> index 5b37790..44fb38c 100644
> --- a/accfg/libaccel_config.h
> +++ b/accfg/libaccel_config.h
> @@ -183,7 +183,6 @@ enum accfg_device_state accfg_device_get_state(struct accfg_device *device);
>   unsigned int accfg_device_get_max_tokens(struct accfg_device *device);
>   unsigned int accfg_device_get_max_batch_size(struct accfg_device *device);
>   unsigned int accfg_device_get_token_limit(struct accfg_device *device);
> -unsigned int accfg_device_get_cdev_major(struct accfg_device *device);
>   unsigned int accfg_device_get_version(struct accfg_device *device);
>   int accfg_device_get_clients(struct accfg_device *device);
>   int accfg_device_set_token_limit(struct accfg_device *dev, int val);
> @@ -258,7 +257,6 @@ int accfg_wq_get_priority(struct accfg_wq *wq);
>   unsigned int accfg_wq_get_priv(struct accfg_wq *wq);
>   int accfg_wq_get_block_on_fault(struct accfg_wq *wq);
>   enum accfg_wq_state accfg_wq_get_state(struct accfg_wq *wq);
> -int accfg_wq_get_cdev_minor(struct accfg_wq *wq);
>   const char *accfg_wq_get_type_name(struct accfg_wq *wq);
>   enum accfg_wq_type accfg_wq_get_type(struct accfg_wq *wq);
>   unsigned int accfg_wq_get_max_batch_size(struct accfg_wq *wq);
> diff --git a/util/json.c b/util/json.c
> index 4fb43d2..d1015c3 100644
> --- a/util/json.c
> +++ b/util/json.c
> @@ -324,11 +324,6 @@ struct json_object *util_device_to_json(struct accfg_device *device,
>   		json_object_object_add(jdevice, "pasid_enabled", jobj);
>   	}
>   
> -	jobj = json_object_new_int(accfg_device_get_cdev_major(device));
> -	if (!jobj)
> -		goto err;
> -	json_object_object_add(jdevice, "cdev_major", jobj);
> -
>   	jobj = json_object_new_int(accfg_device_get_clients(device));
>   	if (!jobj)
>   		goto err;
> @@ -439,12 +434,6 @@ struct json_object *util_wq_to_json(struct accfg_wq *wq,
>   	if (jobj)
>   		json_object_object_add(jaccfg, "max_transfer_size", jobj);
>   
> -	if (!(flags & UTIL_JSON_SAVE) && accfg_wq_get_cdev_minor(wq) >= 0) {
> -		jobj = json_object_new_int(accfg_wq_get_cdev_minor(wq));
> -		if (jobj)
> -			json_object_object_add(jaccfg, "cdev_minor", jobj);
> -	}
> -
>   	jobj = json_object_new_string(wq_type_str[accfg_wq_get_type(wq)]);
>   	if (jobj)
>   		json_object_object_add(jaccfg, "type", jobj);

[Accel-config] Re: [PATCH v1 1/2] accel-config: Remove cdev related code

[Luck, Tony]

[-- Attachment #1: Type: text/plain, Size: 604 bytes --]

> Should we add "cdev path : /dev/dsa/..." to the wq listing?

That's the direction I was thinking (based on the fact that you took away cdev major/minor
and didn't replace them).

But it isn't all that equivalent.  A command line only solution might have had a tough time
getting from the major/minor devices to actually opening a device.

So the answer is maybe ... but only if we can convince ourselves that somebody is
going to use "accel-config list" to feed into some script that extracts the pathname
and hands it onto a process to open/mmap.  That seems something of a stretch.

-Tony

[Accel-config] Re: [PATCH v1 1/2] accel-config: Remove cdev related code

[Thomas, Ramesh]

[-- Attachment #1: Type: text/plain, Size: 468 bytes --]

On Mon, May 17, 2021 at 02:58:37PM, Tony Luck wrote:
> > cdev major/minor method of opening idxd char dev will be deprecated.
> > Currently even in non uacce mode the char dev is opened using the device
> > name at /dev/dsa that is returned by accfg_wq_get_user_dev_path()
> 
> Is there an option to accel-config that will show this device path?

No, it is only returned in the library API. The cdev major/minor was
being shown in wq listing.

> 
> -Tony
>

[Accel-config] Re: [PATCH v1 1/2] accel-config: Remove cdev related code

[Luck, Tony]

[-- Attachment #1: Type: text/plain, Size: 295 bytes --]

> cdev major/minor method of opening idxd char dev will be deprecated.
> Currently even in non uacce mode the char dev is opened using the device
> name at /dev/dsa that is returned by accfg_wq_get_user_dev_path()

Is there an option to accel-config that will show this device path?

-Tony