Message-ID: <99ce6f1f-a8e9-3242-a584-e06756d6c606@pensando.io>
Date:   Mon, 14 Sep 2020 15:02:18 -0700
From:   Shannon Nelson <snelson@...sando.io>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH v3 net-next 2/2] ionic: add devlink firmware update

On 9/10/20 10:56 AM, Jakub Kicinski wrote:
> On Wed, 9 Sep 2020 18:34:57 -0700 Shannon Nelson wrote:
>> On 9/9/20 12:22 PM, Jakub Kicinski wrote:
>>> On Wed, 9 Sep 2020 10:58:19 -0700 Shannon Nelson wrote:
>>>> I'm suggesting that this implementation using the existing devlink
>>>> logging services should suffice until someone can design, implement, and
>>>> get accepted a different bit of plumbing.  Unfortunately, that's not a
>>>> job that I can get to right now.
>>> This hack is too nasty to be accepted.
>> Your comment earlier was
>>
>>   > I wonder if we can steal a page from systemd's book and display
>>   > "time until timeout", or whatchamacallit, like systemd does when it's
>>   > waiting for processes to quit. All drivers have some timeout set on the
>>   > operation. If users knew the driver sets timeout to n minutes and they
>>   > see the timer ticking up they'd be less likely to think the command has
>>   > hanged..
>>
>> I implemented the loop such that the timeout value was the 100%, and
>> each time through the loop the elapsed time value is sent, so the user
>> gets to see the % value increasing as the wait goes on, in the same way
>> they see the download progress percentage ticking away.
> Right but you said that in most cases the value never goes up to 25min,
> so user will see the value increment from 0 to say 5% very slowly and
> then jump to 100%.
>
>> This is how I approached the stated requirement of user seeing the
>> "timer ticking up", using the existing machinery.  This seems to be
>> how devlink_flash_update_status_notify() is expected to be used, so
>> I'm a little surprised at the critique.
> Sorry, I thought the systemd reference would be clear enough, see the
> screenshot here:
>
> https://images.app.goo.gl/gz1Uwg6mcHEd3D2m7
>
> Systemd prints something link:
>
> bla bla bla (XXs / YYs)
>
> where XX is the timer ticking up, and YY is the timeout value.
>
>>> So to be clear your options are:
>>>    - plumb the single extra netlink parameter through to devlink
>>>    - wait for someone else to do that for you, before you get
>>> firmware flashing accepted upstream.
>>>   
>> Since you seem to have something else in mind, a little more detail
>> would be helpful.
>>
>> We currently see devlink updating a percentage, something like:
>> Downloading:  56%
>> using backspaces to overwrite the value as the updates are published.
>>
>> How do you envision the userland interpretation of the timeout
>> ticking? Do you want to see something like:
>> Installing - timeout seconds:  23
>> as a countdown?
> I was under the impression that the systemd format would be familiar
> to users, hence:
>
> Downloading:  56% (Xm Ys / Zm Vz)
>
> The part in brackets only appearing after a few seconds without a
> notification, otherwise the whole thing would get noisy.
>
>> So, maybe a flag parameter that can tell the UI to use the raw value
>> and not massage it into a percentage?
>>
>> Do you see this new netlink parameter to be a boolean switch between
>> the percentage and raw, or maybe a bitflag parameter that might end
>> up with several bits of context information for userland to interpret?
>>
>> Are you thinking of a new flags parameter in
>> devlink_flash_update_status_notify(), or a new function to service
>> this?
>>
>> If a new parameter to devlink_flash_update_status_notify(), maybe it
>> is time to make a struct for flash update data rather than adding
>> more parameters to the function?
>>
>> Should we add yet another parameter to replace the '%' with some
>> other label, so devlink could print something like
>> Installing - timeout in:  23 secs
>>
>> Or could we use a 0 value for total to signify using a raw value and
>> not need to plumb a new parameter?  Although this might not get along
>> well with older devlink utilities.
> I was thinking of adding an extra timeout parameter to
> devlink_flash_update_status_notify() - timeout length in seconds.
> And an extra netlink attr for that.
>
> We could perhaps make:
>
> static inline void
> devlink_flash_update_status_notify( const char *status_msg,
> 				    unsigned long done, unsigned long total)
> {
> 	struct ..._args = {
> 		.status_msg = status_msg,
> 		.done = done,
> 		.total = total,
> 	}
>
> 	__devlink_flash_update_status_notify(devlink, &.._args);
> }
>
> IOW drop the component parameter from the normal helper, cause almost
> nobody uses that. The add a more full featured __ version, which would
> take the arg struct, the struct would include the timeout value.
>
> If the timeout is lower than 15sec drivers will probably have little
> value in reporting it, so simplified helper should be nice there to save LOC.
>
> The user space can do the counting up trivially using select(),
> or a syscall timeout. The netlink notification would only carry timeout.
> (LMK if this is problematic, I haven't looked at the user space part.)

What if we simplify this idea to adding a timeout variant of the 
devlink_flash_update_begin_notify()?  Perhaps something like
     devlink_flash_update_begin_notify_timeout(struct devlink *devlink, 
unsigned int timeout_seconds)
This can pass up a timeout parameter at the beginning of the flash and 
the userland utility can do what it needs at that point to set up the UI 
display.

I think using a struct internally to devlink.c/h might still have merit, 
but I'm not sure there's a need to mess with the rest of the API just yet.

sln

Powered by blists - more mailing lists