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

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.