[Accel-config] Re: [RFC] Provide library API to read and write raw sysfs attributes

[Accel-config] Re: [RFC] Provide library API to read and write raw sysfs attributes

[Ramesh Thomas]

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

On 2/4/2022 3:26 PM, Dave Jiang wrote:
> 
> On 2/4/2022 2:03 PM, Ramesh Thomas wrote:
>> On 2/4/2022 12:42 PM, Dave Jiang wrote:
>>>
>>> On 2/4/2022 12:15 PM, Ramesh Thomas wrote:
>>>> I am thinking of providing some generic functions to read and write 
>>>> sysfs attributes without much processing. This will be useful where 
>>>> raw reads and writes are required e.g. listing values of attributes 
>>>> or initializing attributes in a loop. This will open up 
>>>> possibilities of advanced use cases where user can do custom batch 
>>>> configurations in addition to the load and save configuration option.
>>>>
>>>> There will be a get and set function for devices, groups, wqs and 
>>>> engines as follows
>>>>
>>>> /* structure to read and write attributes from sysfs */
>>>> struct accfg_sysfs_attr {
>>>>         char attr[MAX_PARAM_LEN];
>>>> };
>>>>
>>>> int accfg_get_device_attr(struct accfg_device *device, const char 
>>>> *attr_name,
>>>>                 struct *accfg_sysfs_attr);
>>>> int accfg_set_device_attr(struct accfg_device *device, const char 
>>>> *attr_name,
>>>>                 struct *accfg_sysfs_attr);
>>>>
>>>> int accfg_get_group_attr(struct accfg_group *group, const char 
>>>> *attr_name,
>>>>                 struct *accfg_sysfs_attr);
>>>> int accfg_set_group_attr(struct accfg_group *group, const char 
>>>> *attr_name,
>>>>                 struct *accfg_sysfs_attr);
>>>>
>>> Does it provide additional processing on top of sysfs_read_attr() 
>>> helper function?
>>
>> It creates the path from from device, wq, engine and group structure. 
>> It will save last error (cmd_status) in accfg ctxt and return errors 
>> returned from driver. Other than that it does not do any attribute 
>> specific processing.
> 
> Ok by me from high level view if it makes the code cleaner.

Unfortunately this will not work. The intention was to provide a generic 
API to read/write sysfs attributes as strings. However since we cache 
attributes in their respective types (int, long, string etc.), attribute 
specific processing cannot be avoided.

Maybe in future we could simplify things by not caching values in 
private structures. I don't see a reason to cache them when that is done 
in the sysfs and persisted in config files. We only need to validate 
when user enters a value and load all of them in json as strings.

> 
> 
>>
>>>
>>>
>>>> similarly for wqs and engines.
>>>>
>>>> Let me know if you see any issue or other suggestions.
>>>>
>>>> Thanks,
>>>> Ramesh
>>

[Accel-config] Re: [RFC] Provide library API to read and write raw sysfs attributes

[Ramesh Thomas]

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

On 2/7/2022 4:49 PM, Dave Jiang wrote:
> 
> On 2/7/2022 5:38 PM, Ramesh Thomas wrote:
>> On 2/7/2022 3:58 PM, Dave Jiang wrote:
>>>
>>> On 2/7/2022 4:46 PM, Ramesh Thomas wrote:
>>>> On 2/7/2022 3:24 PM, Dave Jiang wrote:
>>>>>
>>>>> On 2/7/2022 4:18 PM, Ramesh Thomas wrote:
>>>>>> On 2/7/2022 3:05 PM, Dave Jiang wrote:
>>>>>>>
>>>>>>> On 2/7/2022 2:54 PM, Ramesh Thomas wrote:
>>>>>>>> On 2/7/2022 1:43 PM, Dave Jiang wrote:
>>>>>>>>>
>>>>>>>>> On 2/7/2022 2:34 PM, Ramesh Thomas wrote:
>>>>>>>>>> On 2/4/2022 3:26 PM, Dave Jiang wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 2/4/2022 2:03 PM, Ramesh Thomas wrote:
>>>>>>>>>>>> On 2/4/2022 12:42 PM, Dave Jiang wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 2/4/2022 12:15 PM, Ramesh Thomas wrote:
>>>>>>>>>>>>>> I am thinking of providing some generic functions to read 
>>>>>>>>>>>>>> and write sysfs attributes without much processing. This 
>>>>>>>>>>>>>> will be useful where raw reads and writes are required 
>>>>>>>>>>>>>> e.g. listing values of attributes or initializing 
>>>>>>>>>>>>>> attributes in a loop. This will open up possibilities of 
>>>>>>>>>>>>>> advanced use cases where user can do custom batch 
>>>>>>>>>>>>>> configurations in addition to the load and save 
>>>>>>>>>>>>>> configuration option.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> There will be a get and set function for devices, groups, 
>>>>>>>>>>>>>> wqs and engines as follows
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> /* structure to read and write attributes from sysfs */
>>>>>>>>>>>>>> struct accfg_sysfs_attr {
>>>>>>>>>>>>>>         char attr[MAX_PARAM_LEN];
>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> int accfg_get_device_attr(struct accfg_device *device, 
>>>>>>>>>>>>>> const char *attr_name,
>>>>>>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>>>>>>> int accfg_set_device_attr(struct accfg_device *device, 
>>>>>>>>>>>>>> const char *attr_name,
>>>>>>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> int accfg_get_group_attr(struct accfg_group *group, const 
>>>>>>>>>>>>>> char *attr_name,
>>>>>>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>>>>>>> int accfg_set_group_attr(struct accfg_group *group, const 
>>>>>>>>>>>>>> char *attr_name,
>>>>>>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>>>>>>>
>>>>>>>>>>>>> Does it provide additional processing on top of 
>>>>>>>>>>>>> sysfs_read_attr() helper function?
>>>>>>>>>>>>
>>>>>>>>>>>> It creates the path from from device, wq, engine and group 
>>>>>>>>>>>> structure. It will save last error (cmd_status) in accfg 
>>>>>>>>>>>> ctxt and return errors returned from driver. Other than that 
>>>>>>>>>>>> it does not do any attribute specific processing.
>>>>>>>>>>>
>>>>>>>>>>> Ok by me from high level view if it makes the code cleaner.
>>>>>>>>>>
>>>>>>>>>> Unfortunately this will not work. The intention was to provide 
>>>>>>>>>> a generic API to read/write sysfs attributes as strings. 
>>>>>>>>>> However since we cache attributes in their respective types 
>>>>>>>>>> (int, long, string etc.), attribute specific processing cannot 
>>>>>>>>>> be avoided.
>>>>>>>>>>
>>>>>>>>>> Maybe in future we could simplify things by not caching values 
>>>>>>>>>> in private structures. I don't see a reason to cache them when 
>>>>>>>>>> that is done in the sysfs and persisted in config files. We 
>>>>>>>>>> only need to validate when user enters a value and load all of 
>>>>>>>>>> them in json as strings.
>>>>>>>>>
>>>>>>>>> I think the issue is with apps using the libaccel-config API. 
>>>>>>>>> If an attribute is persistent, then you don't want to keep 
>>>>>>>>> pulling it from sysfs when you can read it locally. Especially 
>>>>>>>>> if something is needed for the fast I/O path.
>>>>>>>>
>>>>>>>> Since this is only a configuration tool, there is no need for 
>>>>>>>> fast I/O. We can store all the attributes in json as strings.
>>>>>>>
>>>>>>> That is only for the front end. If something like DML is calling 
>>>>>>> libaccel-config, and it asks for some wq attribute or device 
>>>>>>> attribute it potentially could be calling from somewhere in the 
>>>>>>> fast path.
>>>>>>
>>>>>> Libaccel-config has to stay in sync with syfs so I am not sure how 
>>>>>> can you avoid accessing it. Still we don't need private data 
>>>>>> structures to avoid reading sysfs. Json can store the attributes 
>>>>>> as strings.
>>>>>
>>>>> For changeable attributes yes. But for static attributes no. Just 
>>>>> for a hypothetical example, DML asks for the gencap of a device to 
>>>>> compare permission. This value should be cached. There's no reason 
>>>>> to re-read it at all from sysfs.
>>>>
>>>> Most of the configurations are changeable and even the 
>>>> non-changeable ones would need to be read once at least. My main 
>>>> point is we can just store them as strings in json instead of 
>>>> maintaining the interpreted values in private data structures.
>>> I'm confused about the json part. Isn't json only involved when we 
>>> are reading/writing the config files?
>>
>> Currently we create and load json object with the data only during 
>> config load/save and listing. I was thinking we can use the json 
>> database all the time. That would require querying json when we need 
>> to locate data. If we don't want to use json, we can still store in 
>> data structures like we do currently but as raw strings. That way only 
>> the API that deals with each attribute has to deal with the type and 
>> the storage only mirrors the sysfs.
> 
> I suppose that's up to you. Whatever makes the code cleaner and simpler.

It is a proposal for future improvements depending on priorities. 
Simplifying things would make it easier to maintain.

[Accel-config] Re: [RFC] Provide library API to read and write raw sysfs attributes

[Dave Jiang]

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


On 2/7/2022 5:38 PM, Ramesh Thomas wrote:
> On 2/7/2022 3:58 PM, Dave Jiang wrote:
>>
>> On 2/7/2022 4:46 PM, Ramesh Thomas wrote:
>>> On 2/7/2022 3:24 PM, Dave Jiang wrote:
>>>>
>>>> On 2/7/2022 4:18 PM, Ramesh Thomas wrote:
>>>>> On 2/7/2022 3:05 PM, Dave Jiang wrote:
>>>>>>
>>>>>> On 2/7/2022 2:54 PM, Ramesh Thomas wrote:
>>>>>>> On 2/7/2022 1:43 PM, Dave Jiang wrote:
>>>>>>>>
>>>>>>>> On 2/7/2022 2:34 PM, Ramesh Thomas wrote:
>>>>>>>>> On 2/4/2022 3:26 PM, Dave Jiang wrote:
>>>>>>>>>>
>>>>>>>>>> On 2/4/2022 2:03 PM, Ramesh Thomas wrote:
>>>>>>>>>>> On 2/4/2022 12:42 PM, Dave Jiang wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On 2/4/2022 12:15 PM, Ramesh Thomas wrote:
>>>>>>>>>>>>> I am thinking of providing some generic functions to read 
>>>>>>>>>>>>> and write sysfs attributes without much processing. This 
>>>>>>>>>>>>> will be useful where raw reads and writes are required 
>>>>>>>>>>>>> e.g. listing values of attributes or initializing 
>>>>>>>>>>>>> attributes in a loop. This will open up possibilities of 
>>>>>>>>>>>>> advanced use cases where user can do custom batch 
>>>>>>>>>>>>> configurations in addition to the load and save 
>>>>>>>>>>>>> configuration option.
>>>>>>>>>>>>>
>>>>>>>>>>>>> There will be a get and set function for devices, groups, 
>>>>>>>>>>>>> wqs and engines as follows
>>>>>>>>>>>>>
>>>>>>>>>>>>> /* structure to read and write attributes from sysfs */
>>>>>>>>>>>>> struct accfg_sysfs_attr {
>>>>>>>>>>>>>         char attr[MAX_PARAM_LEN];
>>>>>>>>>>>>> };
>>>>>>>>>>>>>
>>>>>>>>>>>>> int accfg_get_device_attr(struct accfg_device *device, 
>>>>>>>>>>>>> const char *attr_name,
>>>>>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>>>>>> int accfg_set_device_attr(struct accfg_device *device, 
>>>>>>>>>>>>> const char *attr_name,
>>>>>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>>>>>>
>>>>>>>>>>>>> int accfg_get_group_attr(struct accfg_group *group, const 
>>>>>>>>>>>>> char *attr_name,
>>>>>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>>>>>> int accfg_set_group_attr(struct accfg_group *group, const 
>>>>>>>>>>>>> char *attr_name,
>>>>>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>>>>>>
>>>>>>>>>>>> Does it provide additional processing on top of 
>>>>>>>>>>>> sysfs_read_attr() helper function?
>>>>>>>>>>>
>>>>>>>>>>> It creates the path from from device, wq, engine and group 
>>>>>>>>>>> structure. It will save last error (cmd_status) in accfg 
>>>>>>>>>>> ctxt and return errors returned from driver. Other than that 
>>>>>>>>>>> it does not do any attribute specific processing.
>>>>>>>>>>
>>>>>>>>>> Ok by me from high level view if it makes the code cleaner.
>>>>>>>>>
>>>>>>>>> Unfortunately this will not work. The intention was to provide 
>>>>>>>>> a generic API to read/write sysfs attributes as strings. 
>>>>>>>>> However since we cache attributes in their respective types 
>>>>>>>>> (int, long, string etc.), attribute specific processing cannot 
>>>>>>>>> be avoided.
>>>>>>>>>
>>>>>>>>> Maybe in future we could simplify things by not caching values 
>>>>>>>>> in private structures. I don't see a reason to cache them when 
>>>>>>>>> that is done in the sysfs and persisted in config files. We 
>>>>>>>>> only need to validate when user enters a value and load all of 
>>>>>>>>> them in json as strings.
>>>>>>>>
>>>>>>>> I think the issue is with apps using the libaccel-config API. 
>>>>>>>> If an attribute is persistent, then you don't want to keep 
>>>>>>>> pulling it from sysfs when you can read it locally. Especially 
>>>>>>>> if something is needed for the fast I/O path.
>>>>>>>
>>>>>>> Since this is only a configuration tool, there is no need for 
>>>>>>> fast I/O. We can store all the attributes in json as strings.
>>>>>>
>>>>>> That is only for the front end. If something like DML is calling 
>>>>>> libaccel-config, and it asks for some wq attribute or device 
>>>>>> attribute it potentially could be calling from somewhere in the 
>>>>>> fast path.
>>>>>
>>>>> Libaccel-config has to stay in sync with syfs so I am not sure how 
>>>>> can you avoid accessing it. Still we don't need private data 
>>>>> structures to avoid reading sysfs. Json can store the attributes 
>>>>> as strings.
>>>>
>>>> For changeable attributes yes. But for static attributes no. Just 
>>>> for a hypothetical example, DML asks for the gencap of a device to 
>>>> compare permission. This value should be cached. There's no reason 
>>>> to re-read it at all from sysfs.
>>>
>>> Most of the configurations are changeable and even the 
>>> non-changeable ones would need to be read once at least. My main 
>>> point is we can just store them as strings in json instead of 
>>> maintaining the interpreted values in private data structures.
>> I'm confused about the json part. Isn't json only involved when we 
>> are reading/writing the config files?
>
> Currently we create and load json object with the data only during 
> config load/save and listing. I was thinking we can use the json 
> database all the time. That would require querying json when we need 
> to locate data. If we don't want to use json, we can still store in 
> data structures like we do currently but as raw strings. That way only 
> the API that deals with each attribute has to deal with the type and 
> the storage only mirrors the sysfs.

I suppose that's up to you. Whatever makes the code cleaner and simpler.

[Accel-config] Re: [RFC] Provide library API to read and write raw sysfs attributes

[Ramesh Thomas]

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

On 2/7/2022 3:58 PM, Dave Jiang wrote:
> 
> On 2/7/2022 4:46 PM, Ramesh Thomas wrote:
>> On 2/7/2022 3:24 PM, Dave Jiang wrote:
>>>
>>> On 2/7/2022 4:18 PM, Ramesh Thomas wrote:
>>>> On 2/7/2022 3:05 PM, Dave Jiang wrote:
>>>>>
>>>>> On 2/7/2022 2:54 PM, Ramesh Thomas wrote:
>>>>>> On 2/7/2022 1:43 PM, Dave Jiang wrote:
>>>>>>>
>>>>>>> On 2/7/2022 2:34 PM, Ramesh Thomas wrote:
>>>>>>>> On 2/4/2022 3:26 PM, Dave Jiang wrote:
>>>>>>>>>
>>>>>>>>> On 2/4/2022 2:03 PM, Ramesh Thomas wrote:
>>>>>>>>>> On 2/4/2022 12:42 PM, Dave Jiang wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 2/4/2022 12:15 PM, Ramesh Thomas wrote:
>>>>>>>>>>>> I am thinking of providing some generic functions to read 
>>>>>>>>>>>> and write sysfs attributes without much processing. This 
>>>>>>>>>>>> will be useful where raw reads and writes are required e.g. 
>>>>>>>>>>>> listing values of attributes or initializing attributes in a 
>>>>>>>>>>>> loop. This will open up possibilities of advanced use cases 
>>>>>>>>>>>> where user can do custom batch configurations in addition to 
>>>>>>>>>>>> the load and save configuration option.
>>>>>>>>>>>>
>>>>>>>>>>>> There will be a get and set function for devices, groups, 
>>>>>>>>>>>> wqs and engines as follows
>>>>>>>>>>>>
>>>>>>>>>>>> /* structure to read and write attributes from sysfs */
>>>>>>>>>>>> struct accfg_sysfs_attr {
>>>>>>>>>>>>         char attr[MAX_PARAM_LEN];
>>>>>>>>>>>> };
>>>>>>>>>>>>
>>>>>>>>>>>> int accfg_get_device_attr(struct accfg_device *device, const 
>>>>>>>>>>>> char *attr_name,
>>>>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>>>>> int accfg_set_device_attr(struct accfg_device *device, const 
>>>>>>>>>>>> char *attr_name,
>>>>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>>>>>
>>>>>>>>>>>> int accfg_get_group_attr(struct accfg_group *group, const 
>>>>>>>>>>>> char *attr_name,
>>>>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>>>>> int accfg_set_group_attr(struct accfg_group *group, const 
>>>>>>>>>>>> char *attr_name,
>>>>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>>>>>
>>>>>>>>>>> Does it provide additional processing on top of 
>>>>>>>>>>> sysfs_read_attr() helper function?
>>>>>>>>>>
>>>>>>>>>> It creates the path from from device, wq, engine and group 
>>>>>>>>>> structure. It will save last error (cmd_status) in accfg ctxt 
>>>>>>>>>> and return errors returned from driver. Other than that it 
>>>>>>>>>> does not do any attribute specific processing.
>>>>>>>>>
>>>>>>>>> Ok by me from high level view if it makes the code cleaner.
>>>>>>>>
>>>>>>>> Unfortunately this will not work. The intention was to provide a 
>>>>>>>> generic API to read/write sysfs attributes as strings. However 
>>>>>>>> since we cache attributes in their respective types (int, long, 
>>>>>>>> string etc.), attribute specific processing cannot be avoided.
>>>>>>>>
>>>>>>>> Maybe in future we could simplify things by not caching values 
>>>>>>>> in private structures. I don't see a reason to cache them when 
>>>>>>>> that is done in the sysfs and persisted in config files. We only 
>>>>>>>> need to validate when user enters a value and load all of them 
>>>>>>>> in json as strings.
>>>>>>>
>>>>>>> I think the issue is with apps using the libaccel-config API. If 
>>>>>>> an attribute is persistent, then you don't want to keep pulling 
>>>>>>> it from sysfs when you can read it locally. Especially if 
>>>>>>> something is needed for the fast I/O path.
>>>>>>
>>>>>> Since this is only a configuration tool, there is no need for fast 
>>>>>> I/O. We can store all the attributes in json as strings.
>>>>>
>>>>> That is only for the front end. If something like DML is calling 
>>>>> libaccel-config, and it asks for some wq attribute or device 
>>>>> attribute it potentially could be calling from somewhere in the 
>>>>> fast path.
>>>>
>>>> Libaccel-config has to stay in sync with syfs so I am not sure how 
>>>> can you avoid accessing it. Still we don't need private data 
>>>> structures to avoid reading sysfs. Json can store the attributes as 
>>>> strings.
>>>
>>> For changeable attributes yes. But for static attributes no. Just for 
>>> a hypothetical example, DML asks for the gencap of a device to 
>>> compare permission. This value should be cached. There's no reason to 
>>> re-read it at all from sysfs.
>>
>> Most of the configurations are changeable and even the non-changeable 
>> ones would need to be read once at least. My main point is we can just 
>> store them as strings in json instead of maintaining the interpreted 
>> values in private data structures.
> I'm confused about the json part. Isn't json only involved when we are 
> reading/writing the config files?

Currently we create and load json object with the data only during 
config load/save and listing. I was thinking we can use the json 
database all the time. That would require querying json when we need to 
locate data. If we don't want to use json, we can still store in data 
structures like we do currently but as raw strings. That way only the 
API that deals with each attribute has to deal with the type and the 
storage only mirrors the sysfs.

[Accel-config] Re: [RFC] Provide library API to read and write raw sysfs attributes

[Dave Jiang]

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


On 2/7/2022 4:46 PM, Ramesh Thomas wrote:
> On 2/7/2022 3:24 PM, Dave Jiang wrote:
>>
>> On 2/7/2022 4:18 PM, Ramesh Thomas wrote:
>>> On 2/7/2022 3:05 PM, Dave Jiang wrote:
>>>>
>>>> On 2/7/2022 2:54 PM, Ramesh Thomas wrote:
>>>>> On 2/7/2022 1:43 PM, Dave Jiang wrote:
>>>>>>
>>>>>> On 2/7/2022 2:34 PM, Ramesh Thomas wrote:
>>>>>>> On 2/4/2022 3:26 PM, Dave Jiang wrote:
>>>>>>>>
>>>>>>>> On 2/4/2022 2:03 PM, Ramesh Thomas wrote:
>>>>>>>>> On 2/4/2022 12:42 PM, Dave Jiang wrote:
>>>>>>>>>>
>>>>>>>>>> On 2/4/2022 12:15 PM, Ramesh Thomas wrote:
>>>>>>>>>>> I am thinking of providing some generic functions to read 
>>>>>>>>>>> and write sysfs attributes without much processing. This 
>>>>>>>>>>> will be useful where raw reads and writes are required e.g. 
>>>>>>>>>>> listing values of attributes or initializing attributes in a 
>>>>>>>>>>> loop. This will open up possibilities of advanced use cases 
>>>>>>>>>>> where user can do custom batch configurations in addition to 
>>>>>>>>>>> the load and save configuration option.
>>>>>>>>>>>
>>>>>>>>>>> There will be a get and set function for devices, groups, 
>>>>>>>>>>> wqs and engines as follows
>>>>>>>>>>>
>>>>>>>>>>> /* structure to read and write attributes from sysfs */
>>>>>>>>>>> struct accfg_sysfs_attr {
>>>>>>>>>>>         char attr[MAX_PARAM_LEN];
>>>>>>>>>>> };
>>>>>>>>>>>
>>>>>>>>>>> int accfg_get_device_attr(struct accfg_device *device, const 
>>>>>>>>>>> char *attr_name,
>>>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>>>> int accfg_set_device_attr(struct accfg_device *device, const 
>>>>>>>>>>> char *attr_name,
>>>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>>>>
>>>>>>>>>>> int accfg_get_group_attr(struct accfg_group *group, const 
>>>>>>>>>>> char *attr_name,
>>>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>>>> int accfg_set_group_attr(struct accfg_group *group, const 
>>>>>>>>>>> char *attr_name,
>>>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>>>>
>>>>>>>>>> Does it provide additional processing on top of 
>>>>>>>>>> sysfs_read_attr() helper function?
>>>>>>>>>
>>>>>>>>> It creates the path from from device, wq, engine and group 
>>>>>>>>> structure. It will save last error (cmd_status) in accfg ctxt 
>>>>>>>>> and return errors returned from driver. Other than that it 
>>>>>>>>> does not do any attribute specific processing.
>>>>>>>>
>>>>>>>> Ok by me from high level view if it makes the code cleaner.
>>>>>>>
>>>>>>> Unfortunately this will not work. The intention was to provide a 
>>>>>>> generic API to read/write sysfs attributes as strings. However 
>>>>>>> since we cache attributes in their respective types (int, long, 
>>>>>>> string etc.), attribute specific processing cannot be avoided.
>>>>>>>
>>>>>>> Maybe in future we could simplify things by not caching values 
>>>>>>> in private structures. I don't see a reason to cache them when 
>>>>>>> that is done in the sysfs and persisted in config files. We only 
>>>>>>> need to validate when user enters a value and load all of them 
>>>>>>> in json as strings.
>>>>>>
>>>>>> I think the issue is with apps using the libaccel-config API. If 
>>>>>> an attribute is persistent, then you don't want to keep pulling 
>>>>>> it from sysfs when you can read it locally. Especially if 
>>>>>> something is needed for the fast I/O path.
>>>>>
>>>>> Since this is only a configuration tool, there is no need for fast 
>>>>> I/O. We can store all the attributes in json as strings.
>>>>
>>>> That is only for the front end. If something like DML is calling 
>>>> libaccel-config, and it asks for some wq attribute or device 
>>>> attribute it potentially could be calling from somewhere in the 
>>>> fast path.
>>>
>>> Libaccel-config has to stay in sync with syfs so I am not sure how 
>>> can you avoid accessing it. Still we don't need private data 
>>> structures to avoid reading sysfs. Json can store the attributes as 
>>> strings.
>>
>> For changeable attributes yes. But for static attributes no. Just for 
>> a hypothetical example, DML asks for the gencap of a device to 
>> compare permission. This value should be cached. There's no reason to 
>> re-read it at all from sysfs.
>
> Most of the configurations are changeable and even the non-changeable 
> ones would need to be read once at least. My main point is we can just 
> store them as strings in json instead of maintaining the interpreted 
> values in private data structures.
I'm confused about the json part. Isn't json only involved when we are 
reading/writing the config files?

[Accel-config] Re: [RFC] Provide library API to read and write raw sysfs attributes

[Ramesh Thomas]

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

On 2/7/2022 3:24 PM, Dave Jiang wrote:
> 
> On 2/7/2022 4:18 PM, Ramesh Thomas wrote:
>> On 2/7/2022 3:05 PM, Dave Jiang wrote:
>>>
>>> On 2/7/2022 2:54 PM, Ramesh Thomas wrote:
>>>> On 2/7/2022 1:43 PM, Dave Jiang wrote:
>>>>>
>>>>> On 2/7/2022 2:34 PM, Ramesh Thomas wrote:
>>>>>> On 2/4/2022 3:26 PM, Dave Jiang wrote:
>>>>>>>
>>>>>>> On 2/4/2022 2:03 PM, Ramesh Thomas wrote:
>>>>>>>> On 2/4/2022 12:42 PM, Dave Jiang wrote:
>>>>>>>>>
>>>>>>>>> On 2/4/2022 12:15 PM, Ramesh Thomas wrote:
>>>>>>>>>> I am thinking of providing some generic functions to read and 
>>>>>>>>>> write sysfs attributes without much processing. This will be 
>>>>>>>>>> useful where raw reads and writes are required e.g. listing 
>>>>>>>>>> values of attributes or initializing attributes in a loop. 
>>>>>>>>>> This will open up possibilities of advanced use cases where 
>>>>>>>>>> user can do custom batch configurations in addition to the 
>>>>>>>>>> load and save configuration option.
>>>>>>>>>>
>>>>>>>>>> There will be a get and set function for devices, groups, wqs 
>>>>>>>>>> and engines as follows
>>>>>>>>>>
>>>>>>>>>> /* structure to read and write attributes from sysfs */
>>>>>>>>>> struct accfg_sysfs_attr {
>>>>>>>>>>         char attr[MAX_PARAM_LEN];
>>>>>>>>>> };
>>>>>>>>>>
>>>>>>>>>> int accfg_get_device_attr(struct accfg_device *device, const 
>>>>>>>>>> char *attr_name,
>>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>>> int accfg_set_device_attr(struct accfg_device *device, const 
>>>>>>>>>> char *attr_name,
>>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>>>
>>>>>>>>>> int accfg_get_group_attr(struct accfg_group *group, const char 
>>>>>>>>>> *attr_name,
>>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>>> int accfg_set_group_attr(struct accfg_group *group, const char 
>>>>>>>>>> *attr_name,
>>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>>>
>>>>>>>>> Does it provide additional processing on top of 
>>>>>>>>> sysfs_read_attr() helper function?
>>>>>>>>
>>>>>>>> It creates the path from from device, wq, engine and group 
>>>>>>>> structure. It will save last error (cmd_status) in accfg ctxt 
>>>>>>>> and return errors returned from driver. Other than that it does 
>>>>>>>> not do any attribute specific processing.
>>>>>>>
>>>>>>> Ok by me from high level view if it makes the code cleaner.
>>>>>>
>>>>>> Unfortunately this will not work. The intention was to provide a 
>>>>>> generic API to read/write sysfs attributes as strings. However 
>>>>>> since we cache attributes in their respective types (int, long, 
>>>>>> string etc.), attribute specific processing cannot be avoided.
>>>>>>
>>>>>> Maybe in future we could simplify things by not caching values in 
>>>>>> private structures. I don't see a reason to cache them when that 
>>>>>> is done in the sysfs and persisted in config files. We only need 
>>>>>> to validate when user enters a value and load all of them in json 
>>>>>> as strings.
>>>>>
>>>>> I think the issue is with apps using the libaccel-config API. If an 
>>>>> attribute is persistent, then you don't want to keep pulling it 
>>>>> from sysfs when you can read it locally. Especially if something is 
>>>>> needed for the fast I/O path.
>>>>
>>>> Since this is only a configuration tool, there is no need for fast 
>>>> I/O. We can store all the attributes in json as strings.
>>>
>>> That is only for the front end. If something like DML is calling 
>>> libaccel-config, and it asks for some wq attribute or device 
>>> attribute it potentially could be calling from somewhere in the fast 
>>> path.
>>
>> Libaccel-config has to stay in sync with syfs so I am not sure how can 
>> you avoid accessing it. Still we don't need private data structures to 
>> avoid reading sysfs. Json can store the attributes as strings.
> 
> For changeable attributes yes. But for static attributes no. Just for a 
> hypothetical example, DML asks for the gencap of a device to compare 
> permission. This value should be cached. There's no reason to re-read it 
> at all from sysfs.

Most of the configurations are changeable and even the non-changeable 
ones would need to be read once at least. My main point is we can just 
store them as strings in json instead of maintaining the interpreted 
values in private data structures.

[Accel-config] Re: [RFC] Provide library API to read and write raw sysfs attributes

[Dave Jiang]

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


On 2/7/2022 4:18 PM, Ramesh Thomas wrote:
> On 2/7/2022 3:05 PM, Dave Jiang wrote:
>>
>> On 2/7/2022 2:54 PM, Ramesh Thomas wrote:
>>> On 2/7/2022 1:43 PM, Dave Jiang wrote:
>>>>
>>>> On 2/7/2022 2:34 PM, Ramesh Thomas wrote:
>>>>> On 2/4/2022 3:26 PM, Dave Jiang wrote:
>>>>>>
>>>>>> On 2/4/2022 2:03 PM, Ramesh Thomas wrote:
>>>>>>> On 2/4/2022 12:42 PM, Dave Jiang wrote:
>>>>>>>>
>>>>>>>> On 2/4/2022 12:15 PM, Ramesh Thomas wrote:
>>>>>>>>> I am thinking of providing some generic functions to read and 
>>>>>>>>> write sysfs attributes without much processing. This will be 
>>>>>>>>> useful where raw reads and writes are required e.g. listing 
>>>>>>>>> values of attributes or initializing attributes in a loop. 
>>>>>>>>> This will open up possibilities of advanced use cases where 
>>>>>>>>> user can do custom batch configurations in addition to the 
>>>>>>>>> load and save configuration option.
>>>>>>>>>
>>>>>>>>> There will be a get and set function for devices, groups, wqs 
>>>>>>>>> and engines as follows
>>>>>>>>>
>>>>>>>>> /* structure to read and write attributes from sysfs */
>>>>>>>>> struct accfg_sysfs_attr {
>>>>>>>>>         char attr[MAX_PARAM_LEN];
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> int accfg_get_device_attr(struct accfg_device *device, const 
>>>>>>>>> char *attr_name,
>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>> int accfg_set_device_attr(struct accfg_device *device, const 
>>>>>>>>> char *attr_name,
>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>>
>>>>>>>>> int accfg_get_group_attr(struct accfg_group *group, const char 
>>>>>>>>> *attr_name,
>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>> int accfg_set_group_attr(struct accfg_group *group, const char 
>>>>>>>>> *attr_name,
>>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>>
>>>>>>>> Does it provide additional processing on top of 
>>>>>>>> sysfs_read_attr() helper function?
>>>>>>>
>>>>>>> It creates the path from from device, wq, engine and group 
>>>>>>> structure. It will save last error (cmd_status) in accfg ctxt 
>>>>>>> and return errors returned from driver. Other than that it does 
>>>>>>> not do any attribute specific processing.
>>>>>>
>>>>>> Ok by me from high level view if it makes the code cleaner.
>>>>>
>>>>> Unfortunately this will not work. The intention was to provide a 
>>>>> generic API to read/write sysfs attributes as strings. However 
>>>>> since we cache attributes in their respective types (int, long, 
>>>>> string etc.), attribute specific processing cannot be avoided.
>>>>>
>>>>> Maybe in future we could simplify things by not caching values in 
>>>>> private structures. I don't see a reason to cache them when that 
>>>>> is done in the sysfs and persisted in config files. We only need 
>>>>> to validate when user enters a value and load all of them in json 
>>>>> as strings.
>>>>
>>>> I think the issue is with apps using the libaccel-config API. If an 
>>>> attribute is persistent, then you don't want to keep pulling it 
>>>> from sysfs when you can read it locally. Especially if something is 
>>>> needed for the fast I/O path.
>>>
>>> Since this is only a configuration tool, there is no need for fast 
>>> I/O. We can store all the attributes in json as strings.
>>
>> That is only for the front end. If something like DML is calling 
>> libaccel-config, and it asks for some wq attribute or device 
>> attribute it potentially could be calling from somewhere in the fast 
>> path.
>
> Libaccel-config has to stay in sync with syfs so I am not sure how can 
> you avoid accessing it. Still we don't need private data structures to 
> avoid reading sysfs. Json can store the attributes as strings.

For changeable attributes yes. But for static attributes no. Just for a 
hypothetical example, DML asks for the gencap of a device to compare 
permission. This value should be cached. There's no reason to re-read it 
at all from sysfs.

[Accel-config] Re: [RFC] Provide library API to read and write raw sysfs attributes

[Ramesh Thomas]

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

On 2/7/2022 3:05 PM, Dave Jiang wrote:
> 
> On 2/7/2022 2:54 PM, Ramesh Thomas wrote:
>> On 2/7/2022 1:43 PM, Dave Jiang wrote:
>>>
>>> On 2/7/2022 2:34 PM, Ramesh Thomas wrote:
>>>> On 2/4/2022 3:26 PM, Dave Jiang wrote:
>>>>>
>>>>> On 2/4/2022 2:03 PM, Ramesh Thomas wrote:
>>>>>> On 2/4/2022 12:42 PM, Dave Jiang wrote:
>>>>>>>
>>>>>>> On 2/4/2022 12:15 PM, Ramesh Thomas wrote:
>>>>>>>> I am thinking of providing some generic functions to read and 
>>>>>>>> write sysfs attributes without much processing. This will be 
>>>>>>>> useful where raw reads and writes are required e.g. listing 
>>>>>>>> values of attributes or initializing attributes in a loop. This 
>>>>>>>> will open up possibilities of advanced use cases where user can 
>>>>>>>> do custom batch configurations in addition to the load and save 
>>>>>>>> configuration option.
>>>>>>>>
>>>>>>>> There will be a get and set function for devices, groups, wqs 
>>>>>>>> and engines as follows
>>>>>>>>
>>>>>>>> /* structure to read and write attributes from sysfs */
>>>>>>>> struct accfg_sysfs_attr {
>>>>>>>>         char attr[MAX_PARAM_LEN];
>>>>>>>> };
>>>>>>>>
>>>>>>>> int accfg_get_device_attr(struct accfg_device *device, const 
>>>>>>>> char *attr_name,
>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>> int accfg_set_device_attr(struct accfg_device *device, const 
>>>>>>>> char *attr_name,
>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>
>>>>>>>> int accfg_get_group_attr(struct accfg_group *group, const char 
>>>>>>>> *attr_name,
>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>> int accfg_set_group_attr(struct accfg_group *group, const char 
>>>>>>>> *attr_name,
>>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>>
>>>>>>> Does it provide additional processing on top of sysfs_read_attr() 
>>>>>>> helper function?
>>>>>>
>>>>>> It creates the path from from device, wq, engine and group 
>>>>>> structure. It will save last error (cmd_status) in accfg ctxt and 
>>>>>> return errors returned from driver. Other than that it does not do 
>>>>>> any attribute specific processing.
>>>>>
>>>>> Ok by me from high level view if it makes the code cleaner.
>>>>
>>>> Unfortunately this will not work. The intention was to provide a 
>>>> generic API to read/write sysfs attributes as strings. However since 
>>>> we cache attributes in their respective types (int, long, string 
>>>> etc.), attribute specific processing cannot be avoided.
>>>>
>>>> Maybe in future we could simplify things by not caching values in 
>>>> private structures. I don't see a reason to cache them when that is 
>>>> done in the sysfs and persisted in config files. We only need to 
>>>> validate when user enters a value and load all of them in json as 
>>>> strings.
>>>
>>> I think the issue is with apps using the libaccel-config API. If an 
>>> attribute is persistent, then you don't want to keep pulling it from 
>>> sysfs when you can read it locally. Especially if something is needed 
>>> for the fast I/O path.
>>
>> Since this is only a configuration tool, there is no need for fast 
>> I/O. We can store all the attributes in json as strings.
> 
> That is only for the front end. If something like DML is calling 
> libaccel-config, and it asks for some wq attribute or device attribute 
> it potentially could be calling from somewhere in the fast path.

Libaccel-config has to stay in sync with syfs so I am not sure how can 
you avoid accessing it. Still we don't need private data structures to 
avoid reading sysfs. Json can store the attributes as strings.

[Accel-config] Re: [RFC] Provide library API to read and write raw sysfs attributes

[Dave Jiang]

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


On 2/7/2022 2:54 PM, Ramesh Thomas wrote:
> On 2/7/2022 1:43 PM, Dave Jiang wrote:
>>
>> On 2/7/2022 2:34 PM, Ramesh Thomas wrote:
>>> On 2/4/2022 3:26 PM, Dave Jiang wrote:
>>>>
>>>> On 2/4/2022 2:03 PM, Ramesh Thomas wrote:
>>>>> On 2/4/2022 12:42 PM, Dave Jiang wrote:
>>>>>>
>>>>>> On 2/4/2022 12:15 PM, Ramesh Thomas wrote:
>>>>>>> I am thinking of providing some generic functions to read and 
>>>>>>> write sysfs attributes without much processing. This will be 
>>>>>>> useful where raw reads and writes are required e.g. listing 
>>>>>>> values of attributes or initializing attributes in a loop. This 
>>>>>>> will open up possibilities of advanced use cases where user can 
>>>>>>> do custom batch configurations in addition to the load and save 
>>>>>>> configuration option.
>>>>>>>
>>>>>>> There will be a get and set function for devices, groups, wqs 
>>>>>>> and engines as follows
>>>>>>>
>>>>>>> /* structure to read and write attributes from sysfs */
>>>>>>> struct accfg_sysfs_attr {
>>>>>>>         char attr[MAX_PARAM_LEN];
>>>>>>> };
>>>>>>>
>>>>>>> int accfg_get_device_attr(struct accfg_device *device, const 
>>>>>>> char *attr_name,
>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>> int accfg_set_device_attr(struct accfg_device *device, const 
>>>>>>> char *attr_name,
>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>
>>>>>>> int accfg_get_group_attr(struct accfg_group *group, const char 
>>>>>>> *attr_name,
>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>> int accfg_set_group_attr(struct accfg_group *group, const char 
>>>>>>> *attr_name,
>>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>>
>>>>>> Does it provide additional processing on top of sysfs_read_attr() 
>>>>>> helper function?
>>>>>
>>>>> It creates the path from from device, wq, engine and group 
>>>>> structure. It will save last error (cmd_status) in accfg ctxt and 
>>>>> return errors returned from driver. Other than that it does not do 
>>>>> any attribute specific processing.
>>>>
>>>> Ok by me from high level view if it makes the code cleaner.
>>>
>>> Unfortunately this will not work. The intention was to provide a 
>>> generic API to read/write sysfs attributes as strings. However since 
>>> we cache attributes in their respective types (int, long, string 
>>> etc.), attribute specific processing cannot be avoided.
>>>
>>> Maybe in future we could simplify things by not caching values in 
>>> private structures. I don't see a reason to cache them when that is 
>>> done in the sysfs and persisted in config files. We only need to 
>>> validate when user enters a value and load all of them in json as 
>>> strings.
>>
>> I think the issue is with apps using the libaccel-config API. If an 
>> attribute is persistent, then you don't want to keep pulling it from 
>> sysfs when you can read it locally. Especially if something is needed 
>> for the fast I/O path.
>
> Since this is only a configuration tool, there is no need for fast 
> I/O. We can store all the attributes in json as strings.

That is only for the front end. If something like DML is calling 
libaccel-config, and it asks for some wq attribute or device attribute 
it potentially could be calling from somewhere in the fast path.


>
>>
>>
>>>
>>>>
>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>> similarly for wqs and engines.
>>>>>>>
>>>>>>> Let me know if you see any issue or other suggestions.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Ramesh
>>>>>
>>>
>

[Accel-config] Re: [RFC] Provide library API to read and write raw sysfs attributes

[Ramesh Thomas]

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

On 2/7/2022 1:43 PM, Dave Jiang wrote:
> 
> On 2/7/2022 2:34 PM, Ramesh Thomas wrote:
>> On 2/4/2022 3:26 PM, Dave Jiang wrote:
>>>
>>> On 2/4/2022 2:03 PM, Ramesh Thomas wrote:
>>>> On 2/4/2022 12:42 PM, Dave Jiang wrote:
>>>>>
>>>>> On 2/4/2022 12:15 PM, Ramesh Thomas wrote:
>>>>>> I am thinking of providing some generic functions to read and 
>>>>>> write sysfs attributes without much processing. This will be 
>>>>>> useful where raw reads and writes are required e.g. listing values 
>>>>>> of attributes or initializing attributes in a loop. This will open 
>>>>>> up possibilities of advanced use cases where user can do custom 
>>>>>> batch configurations in addition to the load and save 
>>>>>> configuration option.
>>>>>>
>>>>>> There will be a get and set function for devices, groups, wqs and 
>>>>>> engines as follows
>>>>>>
>>>>>> /* structure to read and write attributes from sysfs */
>>>>>> struct accfg_sysfs_attr {
>>>>>>         char attr[MAX_PARAM_LEN];
>>>>>> };
>>>>>>
>>>>>> int accfg_get_device_attr(struct accfg_device *device, const char 
>>>>>> *attr_name,
>>>>>>                 struct *accfg_sysfs_attr);
>>>>>> int accfg_set_device_attr(struct accfg_device *device, const char 
>>>>>> *attr_name,
>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>
>>>>>> int accfg_get_group_attr(struct accfg_group *group, const char 
>>>>>> *attr_name,
>>>>>>                 struct *accfg_sysfs_attr);
>>>>>> int accfg_set_group_attr(struct accfg_group *group, const char 
>>>>>> *attr_name,
>>>>>>                 struct *accfg_sysfs_attr);
>>>>>>
>>>>> Does it provide additional processing on top of sysfs_read_attr() 
>>>>> helper function?
>>>>
>>>> It creates the path from from device, wq, engine and group 
>>>> structure. It will save last error (cmd_status) in accfg ctxt and 
>>>> return errors returned from driver. Other than that it does not do 
>>>> any attribute specific processing.
>>>
>>> Ok by me from high level view if it makes the code cleaner.
>>
>> Unfortunately this will not work. The intention was to provide a 
>> generic API to read/write sysfs attributes as strings. However since 
>> we cache attributes in their respective types (int, long, string 
>> etc.), attribute specific processing cannot be avoided.
>>
>> Maybe in future we could simplify things by not caching values in 
>> private structures. I don't see a reason to cache them when that is 
>> done in the sysfs and persisted in config files. We only need to 
>> validate when user enters a value and load all of them in json as 
>> strings.
> 
> I think the issue is with apps using the libaccel-config API. If an 
> attribute is persistent, then you don't want to keep pulling it from 
> sysfs when you can read it locally. Especially if something is needed 
> for the fast I/O path.

Since this is only a configuration tool, there is no need for fast I/O. 
We can store all the attributes in json as strings.

> 
> 
>>
>>>
>>>
>>>>
>>>>>
>>>>>
>>>>>> similarly for wqs and engines.
>>>>>>
>>>>>> Let me know if you see any issue or other suggestions.
>>>>>>
>>>>>> Thanks,
>>>>>> Ramesh
>>>>
>>

[Accel-config] Re: [RFC] Provide library API to read and write raw sysfs attributes

[Dave Jiang]

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


On 2/7/2022 2:34 PM, Ramesh Thomas wrote:
> On 2/4/2022 3:26 PM, Dave Jiang wrote:
>>
>> On 2/4/2022 2:03 PM, Ramesh Thomas wrote:
>>> On 2/4/2022 12:42 PM, Dave Jiang wrote:
>>>>
>>>> On 2/4/2022 12:15 PM, Ramesh Thomas wrote:
>>>>> I am thinking of providing some generic functions to read and 
>>>>> write sysfs attributes without much processing. This will be 
>>>>> useful where raw reads and writes are required e.g. listing values 
>>>>> of attributes or initializing attributes in a loop. This will open 
>>>>> up possibilities of advanced use cases where user can do custom 
>>>>> batch configurations in addition to the load and save 
>>>>> configuration option.
>>>>>
>>>>> There will be a get and set function for devices, groups, wqs and 
>>>>> engines as follows
>>>>>
>>>>> /* structure to read and write attributes from sysfs */
>>>>> struct accfg_sysfs_attr {
>>>>>         char attr[MAX_PARAM_LEN];
>>>>> };
>>>>>
>>>>> int accfg_get_device_attr(struct accfg_device *device, const char 
>>>>> *attr_name,
>>>>>                 struct *accfg_sysfs_attr);
>>>>> int accfg_set_device_attr(struct accfg_device *device, const char 
>>>>> *attr_name,
>>>>>                 struct *accfg_sysfs_attr);
>>>>>
>>>>> int accfg_get_group_attr(struct accfg_group *group, const char 
>>>>> *attr_name,
>>>>>                 struct *accfg_sysfs_attr);
>>>>> int accfg_set_group_attr(struct accfg_group *group, const char 
>>>>> *attr_name,
>>>>>                 struct *accfg_sysfs_attr);
>>>>>
>>>> Does it provide additional processing on top of sysfs_read_attr() 
>>>> helper function?
>>>
>>> It creates the path from from device, wq, engine and group 
>>> structure. It will save last error (cmd_status) in accfg ctxt and 
>>> return errors returned from driver. Other than that it does not do 
>>> any attribute specific processing.
>>
>> Ok by me from high level view if it makes the code cleaner.
>
> Unfortunately this will not work. The intention was to provide a 
> generic API to read/write sysfs attributes as strings. However since 
> we cache attributes in their respective types (int, long, string 
> etc.), attribute specific processing cannot be avoided.
>
> Maybe in future we could simplify things by not caching values in 
> private structures. I don't see a reason to cache them when that is 
> done in the sysfs and persisted in config files. We only need to 
> validate when user enters a value and load all of them in json as 
> strings.

I think the issue is with apps using the libaccel-config API. If an 
attribute is persistent, then you don't want to keep pulling it from 
sysfs when you can read it locally. Especially if something is needed 
for the fast I/O path.


>
>>
>>
>>>
>>>>
>>>>
>>>>> similarly for wqs and engines.
>>>>>
>>>>> Let me know if you see any issue or other suggestions.
>>>>>
>>>>> Thanks,
>>>>> Ramesh
>>>
>

[Accel-config] Re: [RFC] Provide library API to read and write raw sysfs attributes

[Dave Jiang]

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


On 2/4/2022 2:03 PM, Ramesh Thomas wrote:
> On 2/4/2022 12:42 PM, Dave Jiang wrote:
>>
>> On 2/4/2022 12:15 PM, Ramesh Thomas wrote:
>>> I am thinking of providing some generic functions to read and write 
>>> sysfs attributes without much processing. This will be useful where 
>>> raw reads and writes are required e.g. listing values of attributes 
>>> or initializing attributes in a loop. This will open up 
>>> possibilities of advanced use cases where user can do custom batch 
>>> configurations in addition to the load and save configuration option.
>>>
>>> There will be a get and set function for devices, groups, wqs and 
>>> engines as follows
>>>
>>> /* structure to read and write attributes from sysfs */
>>> struct accfg_sysfs_attr {
>>>         char attr[MAX_PARAM_LEN];
>>> };
>>>
>>> int accfg_get_device_attr(struct accfg_device *device, const char 
>>> *attr_name,
>>>                 struct *accfg_sysfs_attr);
>>> int accfg_set_device_attr(struct accfg_device *device, const char 
>>> *attr_name,
>>>                 struct *accfg_sysfs_attr);
>>>
>>> int accfg_get_group_attr(struct accfg_group *group, const char 
>>> *attr_name,
>>>                 struct *accfg_sysfs_attr);
>>> int accfg_set_group_attr(struct accfg_group *group, const char 
>>> *attr_name,
>>>                 struct *accfg_sysfs_attr);
>>>
>> Does it provide additional processing on top of sysfs_read_attr() 
>> helper function?
>
> It creates the path from from device, wq, engine and group structure. 
> It will save last error (cmd_status) in accfg ctxt and return errors 
> returned from driver. Other than that it does not do any attribute 
> specific processing.

Ok by me from high level view if it makes the code cleaner.


>
>>
>>
>>> similarly for wqs and engines.
>>>
>>> Let me know if you see any issue or other suggestions.
>>>
>>> Thanks,
>>> Ramesh
>

[Accel-config] Re: [RFC] Provide library API to read and write raw sysfs attributes

[Ramesh Thomas]

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

On 2/4/2022 12:42 PM, Dave Jiang wrote:
> 
> On 2/4/2022 12:15 PM, Ramesh Thomas wrote:
>> I am thinking of providing some generic functions to read and write 
>> sysfs attributes without much processing. This will be useful where 
>> raw reads and writes are required e.g. listing values of attributes or 
>> initializing attributes in a loop. This will open up possibilities of 
>> advanced use cases where user can do custom batch configurations in 
>> addition to the load and save configuration option.
>>
>> There will be a get and set function for devices, groups, wqs and 
>> engines as follows
>>
>> /* structure to read and write attributes from sysfs */
>> struct accfg_sysfs_attr {
>>         char attr[MAX_PARAM_LEN];
>> };
>>
>> int accfg_get_device_attr(struct accfg_device *device, const char 
>> *attr_name,
>>                 struct *accfg_sysfs_attr);
>> int accfg_set_device_attr(struct accfg_device *device, const char 
>> *attr_name,
>>                 struct *accfg_sysfs_attr);
>>
>> int accfg_get_group_attr(struct accfg_group *group, const char 
>> *attr_name,
>>                 struct *accfg_sysfs_attr);
>> int accfg_set_group_attr(struct accfg_group *group, const char 
>> *attr_name,
>>                 struct *accfg_sysfs_attr);
>>
> Does it provide additional processing on top of sysfs_read_attr() helper 
> function?

It creates the path from from device, wq, engine and group structure. It 
will save last error (cmd_status) in accfg ctxt and return errors 
returned from driver. Other than that it does not do any attribute 
specific processing.

> 
> 
>> similarly for wqs and engines.
>>
>> Let me know if you see any issue or other suggestions.
>>
>> Thanks,
>> Ramesh

[Accel-config] Re: [RFC] Provide library API to read and write raw sysfs attributes

[Dave Jiang]

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


On 2/4/2022 12:15 PM, Ramesh Thomas wrote:
> I am thinking of providing some generic functions to read and write 
> sysfs attributes without much processing. This will be useful where 
> raw reads and writes are required e.g. listing values of attributes or 
> initializing attributes in a loop. This will open up possibilities of 
> advanced use cases where user can do custom batch configurations in 
> addition to the load and save configuration option.
>
> There will be a get and set function for devices, groups, wqs and 
> engines as follows
>
> /* structure to read and write attributes from sysfs */
> struct accfg_sysfs_attr {
>         char attr[MAX_PARAM_LEN];
> };
>
> int accfg_get_device_attr(struct accfg_device *device, const char 
> *attr_name,
>                 struct *accfg_sysfs_attr);
> int accfg_set_device_attr(struct accfg_device *device, const char 
> *attr_name,
>                 struct *accfg_sysfs_attr);
>
> int accfg_get_group_attr(struct accfg_group *group, const char 
> *attr_name,
>                 struct *accfg_sysfs_attr);
> int accfg_set_group_attr(struct accfg_group *group, const char 
> *attr_name,
>                 struct *accfg_sysfs_attr);
>
Does it provide additional processing on top of sysfs_read_attr() helper 
function?


> similarly for wqs and engines.
>
> Let me know if you see any issue or other suggestions.
>
> Thanks,
> Ramesh