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

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?