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

[Dan Gora]

On Fri, Sep 28, 2018 at 5:03 AM Ferruh Yigit <ferruh.yigit at intel.com> wrote:
>
> On 9/28/2018 12:51 AM, Dan Gora wrote:
> > On Thu, Sep 27, 2018 at 8:44 PM, Ferruh Yigit <ferruh.yigit at intel.com> wrote:
> >>> Well, yes the link_status (link up, link down) _is_ applied to the KNI
> >>> interface.  When that occurs, most people want to know what the link
> >>> speed is that the link came up at.
> >>
> >> +1 to this, people would like to know link speed of the interface.
> >> Are you printing link speed of interface? You are printing whatever user pass to
> >> API.
> >
> > There is no such thing as "link speed of the interface".  The link
> > speed is the speed of the underlying Ethernet link that the interface
> > corresponds to.  This is true for all other ethernet interfaces in the
> > kernel.
>
> This is an API to set link status of KNI interface, KNI interface is a virtual
> interface and no need to be backed by a physical interface at all.

Yes, I understand that.

> Only kni sample application uses it in a way to match a physical interface to a
> KNI interface, but please check KNI PMD where you can have multiple KNI
> interface without any physical device at all.

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

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