Message-ID: <20200910105643.2e2d07f8@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 10 Sep 2020 10:56:43 -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 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(struct devlink *devlink, 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.)

Powered by blists - more mailing lists