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

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.