Re: [dpdk-dev] [PATCH v2 1/5] kni: add API to set link status on kernel interface

[Dan Gora <dg@adax.com>]

On Wed, Oct 10, 2018 at 11:09 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> > Yes, I understand that.. As I pointed out previously, if there is no
> > physical device which corresponds to this KNI interface, the
> > application can:
> >
> > 1) Not use this API at all, just as they do now.
>
> This API has nothing with if KNI interface has corresponding physical device or
> not, all KNI users can use it.
>

I don't know what this means.

> > 2) Use the API and set the state to linkup/0Mbps/autoNeg/Full.  This
> > is perfectly reasonable and no one would be confused by this.
>
> That is the think, you are not setting anything, you are just printing
> "0Mbps/autoNeg/Full" and assuming/hoping the _if_ there is a corresponding
> physical device app sends the values of that device to API for printing.

ugh.. Again the API doesn't care if the values are "right".  The
application writer probably does though.


> Overall I am not sure if there is a value to discuss this more, and I don't want
> this relatively small issue to block the patch, I will comment more to latest
> version.

Agreed, I don't know how else I can express myself to get my point across.

Can you at least comment on the rest of this email below that I spent
a lot of time writing, trying to explain what I'm talking about?

> >>>> I guess you trust to user to provide correct values there, but since only link
> >>>> up & down matters, what prevents user to leave other fields, like speed, just
> >>>> random values?
> >>>
> >>> Nothing.  What prevents anyone from providing random values for
> >>> anything?  The point of the API was to make it super simple, just:
> >>>
> >>> rte_eth_link_get_nowait(portid, &link);
> >>> rte_kni_update_link(p[portid]->kni[i], &link);
> >>
> >> You are only thinking about this use case. Normally the input to API should be
> >> verified right, for this case there is no way to verify it and vales are not
> >> used at all, it is just printed in API.
> >
> > In most applications it is useful to have a message printed which
> > shows the *change* in link status as well as the speed that the link
> > came up at.  If you don't provide the link speed, etc information,
> > then the API is not really useful.  Yes the application writer can
> > provide whatever they want for the link speed/duplex/autoneg, but so
> > what?  Their application will have a nonsensical log message.  It's
> > not DPDK's job to enforce sanity on everyone's application.
> >
> > And no, not every input to every API function is verified.  There are
> > lots of examples in the DPDK where this is not the case.  The
> > parameters should be verified to ensure that they do not cause the
> > application to "break", not necessarily so that they make sense
> > (whatever that would mean in this case).
> >
> > It was recommended that we change the rte_kni_update_link() API to
> > only use the link status as an input and print the log messages
> > outside the API in the application code.  However think about what
> > that would entail:
> >
> > You only want to log *changes* in the link state, not log a message
> > when you set the same state.  This means that either:
> >
> > 1) the application would have to store the previous carrier state.
> > 2) rte_kni_update_link could return the previous value read from the
> > /sys/.../carrier file.
> > 3) the application could read the /sys/.../carrier file before calling
> > rte_kni_update_link to read the previous carrier state.
> >
> > In case 1 you're obliging all users of this API to carry around this
> > extra state for no real reason.  The API can easily read the
> > /sys/.../carrier file to see the previous state while it has it open,
> > just as it does in this patch.
> >
> > In case 2, the application can easily detect when the state changes,
> > but then you end up with a return value something like -1 for errors,
> > 0 for previous_link == down, 1 for previous_link == up.  I fail to see
> > how this is a better API design that what has already been proposed,
> > but it's at least somewhat useful, even if awkward.
> >
> > In the DPDK application, you get something like:
> >
> > rte_eth_link_get_nowait(portid, &link);
> > prev_link = rte_kni_update_link(p[portid]->kni[i], link.link_status);
> > if (prev_link >= 0)
> > {
> >    if (prev_link == 0 && link.link_status == 1)
> >      printf("LINKUP: speed %d duplex %s autoneg %s\n", ....)
> >   else if (prev_link == 1 && link.link_status == 0)
> >     printf("LINKDOWN:\n");
> > } else {
> >   printf("Error: something went wrong...\n");
> > }
> >
> > I don't really see how this is better than:
> >
> > rte_eth_link_get_nowait(portid, &link);
> > rte_kni_update_link(p[portid]->kni[i], &link);
> >
> > but that's my opinion...
> >
> > In case 3, you might as well not even have this API since you're
> > duplicating half of the same code.
> >
> > I would be willing to implement case 2, however I still think that it
> > is not as good a design and will unnecessarily complicate application
> > code compared to the current patch here.  Cases 1 and 3 are
> > non-starters for me.  Our application needs log messages only when the
> > link *changes* state.  These log messages need to include the link
> > speed/duplex/autoneg info.  I think that most, if not all,
> > applications would want that too.  It's how every other physical
> > ethernet driver in the linux kernel works.  DPDK applications which
> > use only KNI virtual interfaces without a physical interface are under
> > no obligation to use this API function.
> >
> > thanks
> > dan