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

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.


>
>>
>>
>>>
>>>>
>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>> similarly for wqs and engines.
>>>>>>>
>>>>>>> Let me know if you see any issue or other suggestions.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Ramesh
>>>>>
>>>
>