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

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 10/3/2018 8:07 PM, Dan Gora wrote:
> On Fri, Sep 28, 2018 at 5:03 AM Ferruh Yigit <ferruh.yigit@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@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.

This API has nothing with if KNI interface has corresponding physical device or
not, all KNI users can use it.

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

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.

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