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

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.