Re: [PATCH RFC v6 2/6] dpll: Add DPLL framework base functions

[Jiri Pirko <jiri@resnulli.us>]

Tue, Mar 28, 2023 at 05:22:04PM CEST, vadim.fedorenko@linux.dev wrote:
>On 15/03/2023 09:22, Jiri Pirko wrote:
>> Tue, Mar 14, 2023 at 06:50:57PM CET, arkadiusz.kubalewski@intel.com wrote:
>> > > From: Jiri Pirko <jiri@resnulli.us>
>> > > Sent: Tuesday, March 14, 2023 10:22 AM
>> > > 
>> > > Mon, Mar 13, 2023 at 11:59:32PM CET, vadim.fedorenko@linux.dev wrote:
>> > > > On 13.03.2023 16:21, Jiri Pirko wrote:
>> > > > > Sun, Mar 12, 2023 at 03:28:03AM CET, vadfed@meta.com wrote:

[...]


>> > > > > > +static int
>> > > > > > +dpll_msg_add_dev_handle(struct sk_buff *msg, const struct dpll_device
>> > > > > > *dpll)
>> > > > > > +{
>> > > > > > +	if (nla_put_u32(msg, DPLL_A_ID, dpll->id))
>> > > > > 
>> > > > > Why exactly do we need this dua--handle scheme? Why do you need
>> > > > > unpredictable DPLL_A_ID to be exposed to userspace?
>> > > > > It's just confusing.
>> > > > > 
>> > > > To be able to work with DPLL per integer after iterator on the list deducts
>> > > > which DPLL device is needed. It can reduce the amount of memory copies and
>> > > > simplify comparisons. Not sure why it's confusing.
>> > > 
>> > > Wait, I don't get it. Could you please explain a bit more?
>> > > 
>> > > My point is, there should be not such ID exposed over netlink
>> > > You don't need to expose it to userspace. The user has well defined
>> > > handle as you agreed with above. For example:
>> > > 
>> > > ice/c92d02a7129f4747/1
>> > > ice/c92d02a7129f4747/2
>> > > 
>> > > This is shown in dpll device GET/DUMP outputs.
>> > > Also user passes it during SET operation:
>> > > $ dplltool set ice/c92d02a7129f4747/1 mode auto
>> > > 
>> > > Isn't that enough stable and nice?
>> > > 
>> > 
>> > I agree with Vadim, this is rather to be used by a daemon tools, which
>> > would get the index once, then could use it as long as device is there.
>> 
>> So basically you say, you can have 2 approaches in app:
>> 1)
>> id = dpll_device_get_id("ice/c92d02a7129f4747/1")
>> dpll_device_set(id, something);
>> dpll_device_set(id, something);
>> dpll_device_set(id, something);
>> 2):
>> dpll_device_set("ice/c92d02a7129f4747/1, something);
>> dpll_device_set("ice/c92d02a7129f4747/1, something);
>> dpll_device_set("ice/c92d02a7129f4747/1, something);
>> 
>> What is exactly benefit of the first one? Why to have 2 handles? Devlink
>> is a nice example of 2) approach, no problem there.
>> 
>> Perhaps I'm missing something, but looks like you want the id for no
>> good reason and this dual-handle scheme just makes things more
>> complicated with 0 added value.
>
>I would like to avoid any extra memory copies or memory checks when it's
>possible to compare single u32/u64 index value. I might be invisible on a
>single host setup, but running monitoring at scale which will parse and
>compare string on every get/event can burn a bit of compute capacity.

Wait, that does not make any sense what so ever.
Show me numbers and real usecase. A sane app gets once at start, then
processes notifications. You have a flow where string compare on a
netlink command makes difference to int compare? Like tens of thousands
of netlink cmds per second? I doubt that. If yes, it is a misuse. Btw,
the string compare would be your last problem comparing the overhead of
Netlink processing with ioctl for example.

Your dual handle scheme just adds complexicity, confusion with 0 added
value. Please drop it. I really don't understand the need to defend this
odd approach :/

[...]


>> > > > > > +
>> > > > > > +	return dpll_pre_dumpit(cb);
>> > > > > > +}
>> > > > > > +
>> > > > > > +int dpll_pin_post_dumpit(struct netlink_callback *cb)
>> > > > > > +{
>> > > > > > +	mutex_unlock(&dpll_pin_xa_lock);
>> > > > > > +
>> > > > > > +	return dpll_post_dumpit(cb);
>> > > > > > +}
>> > > > > > +
>> > > > > > +static int
>> > > > > > +dpll_event_device_change(struct sk_buff *msg, struct dpll_device
>> > > > > > *dpll,
>> > > > > > +			 struct dpll_pin *pin, struct dpll_pin *parent,
>> > > > > > +			 enum dplla attr)
>> > > > > > +{
>> > > > > > +	int ret = dpll_msg_add_dev_handle(msg, dpll);
>> > > > > > +	struct dpll_pin_ref *ref = NULL;
>> > > > > > +	enum dpll_pin_state state;
>> > > > > > +
>> > > > > > +	if (ret)
>> > > > > > +		return ret;
>> > > > > > +	if (pin && nla_put_u32(msg, DPLL_A_PIN_IDX, pin-
>> > > > > > dev_driver_id))
>> > > > > > +		return -EMSGSIZE;
>> > > > > 
>> > > > > I don't really understand why you are trying figure something new and
>> > > > > interesting with the change notifications. This object mix and random
>> > > > > attrs fillup is something very wrong and makes userspace completely
>> > > > > fuzzy about what it is getting. And yet it is so simple:
>> > > > > You have 2 objects, dpll and pin, please just have:
>> > > > > dpll_notify()
>> > > > > dpll_pin_notify()
>> > > > > and share the attrs fillup code with pin_get() and dpll_get() callbacks.
>> > > > > No need for any smartness. Have this dumb and simple.
>> > > > > 
>> > > > > Think about it more as about "object-state-snapshot" than "atomic-change"
>> > > > 
>> > > > But with full object-snapshot user space app will lose the information about
>> > > > what exactly has changed. The reason to have this event is to provide the
>> > > > attributes which have changed. Otherwise, the app should have full snapshot
>> > > > and
>> > > > compare all attributes to figure out changes and that's might not be great
>> > > > idea.
>> > > 
>> > > Wait, are you saying that the app is stateless? Could you provide
>> > > example use cases?
>> > > 
>> > >From what I see, the app managing dpll knows the state of the device and
>> > > pins, it monitors for the changes and saves new state with appropriate
>> > > reaction (might be some action or maybe just log entry).
>> > > 
>> > 
>> > It depends on the use case, right? App developer having those information knows
>> > what has changed, thus can react in a way it thinks is most suitable.
>> > IMHO from user perspective it is good to have a notification which actually
>> > shows it's reason, so proper flow could be assigned to handle the reaction.
>> 
>> Again, could you provide me specific example in which this is needed?
>> I may be missing something, but I don't see how it can bring
>> and benefit. It just makes the live of the app harder because it has to
>> treat the get and notify messages differently.
>> 
>> It is quite common for app to:
>> init:
>> 1) get object state
>> 2) store it
>> 3) apply configuration
>> runtime:
>> 1) listen to object state change
>> 2) store it
>> 3) apply configuration
>> 
>> Same code for both.
>
>Well, I'm thinking about simple monitoring app which will wait for the events
>and create an alert if the changes coming from the event differ from the
>"allowed configs". In this case no real reason to store whole object state,
>but the changes in events are very useful.

No problem. But as I wrote elsewhere in this thread, make sure that msg
format of a change message is the same as the format of get cmd message.
Basically think of it as a get cmd message filtered to have only
attrs which changed. Same nesting and everything. Makes it simple for
the app to have same parsing code for get/dump/notification messages.

[...]