Message-ID: <20200914155955.6104c0ec@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 14 Sep 2020 15:59:55 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Shannon Nelson <snelson@...sando.io>
Cc:     netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH v3 net-next 2/2] ionic: add devlink firmware update

On Mon, 14 Sep 2020 15:02:18 -0700 Shannon Nelson wrote:
> 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)

+const char *status_msg, I assume?

> 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.

Do you mean that this will set the timeout on the entire operation? 
And then still trigger notifications with
devlink_flash_update_begin_notify()? I was considering that to avoid
the need to add a new param, but it doesn't match the need all that 
well, most of time the timeout is on a portion of the operation (e.g. 
a FW command), not the entire process.

If you mean that we can just provide a simpler helper to the drivers
and internally fill in @done and @total as 0 - that could be a good 
option. I'd still prefer if there was one devlink_nl_flash_update_fill() 
that gets a structure.

Powered by blists - more mailing lists