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

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.