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

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.