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

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.