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

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

On 9/26/2018 3:55 PM, Dan Gora wrote:
> On Wed, Sep 26, 2018 at 10:59 AM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>> On 9/19/2018 8:55 PM, Dan Gora wrote:
>>> Add a new API function to KNI, rte_kni_update_link() to allow DPDK
>>> applications to update the link status for KNI network interfaces in
>>> the linux kernel.
>>>
>>> Signed-off-by: Dan Gora <dg@adax.com>
>>
>> <...>
>>
>>> +int __rte_experimental
>>> +rte_kni_update_link(struct rte_kni *kni, struct rte_eth_link *link)
>>> +{
>>> +     char path[64];
>>> +     char carrier[2];
>>> +     const char *new_carrier;
>>> +     int fd, ret;
>>> +
>>> +     if (kni == NULL || link == NULL)
>>> +             return -1;
>>> +
>>> +     snprintf(path, sizeof(path), "/sys/devices/virtual/net/%s/carrier",
>>> +             kni->name);
>>> +
>>> +     fd = open(path, O_RDWR);
>>> +     if (fd == -1) {
>>> +             RTE_LOG(ERR, KNI, "Failed to open file: %s.\n", path);
>>> +             return -1;
>>> +     }
>>> +
>>> +     ret = read(fd, carrier, 2);
>>> +     if (ret < 1) {
>>> +             /* Cannot read carrier until interface is marked
>>> +              * 'up', so don't log an error.
>>> +              */
>>> +             close(fd);
>>> +             return -1;
>>> +     }
>>> +
>>> +     new_carrier = (link->link_status == ETH_LINK_UP) ? "1" : "0";
>>> +     ret = write(fd, new_carrier, 1);
>>> +     if (ret < 1) {
>>> +             RTE_LOG(ERR, KNI, "Failed to write file: %s.\n", path);
>>> +             close(fd);
>>> +             return -1;
>>> +     }
>>> +
>>> +     if (strncmp(carrier, new_carrier, 1)) {
>>> +             if (link->link_status == ETH_LINK_UP) {
>>> +                     RTE_LOG(INFO, KNI, "%s NIC Link is Up %d Mbps %s %s.\n",
>>> +                             kni->name,
>>> +                             link->link_speed,
>>> +                             link->link_autoneg ? "(AutoNeg)" : "(Fixed)",
>>> +                             link->link_duplex ?
>>> +                                     "Full Duplex" : "Half Duplex");
>>
>> These link values are coming from user and not reflected to the kni interface,
>> printing them here can cause a misunderstanding that they have been applied.
>> I think only link status should be printed here.
>>
> 
> There is nothing to "reflect" to the kernel interface, nor to apply to
> the kernel interface.  This is exactly how every other kernel driver
> works on link status changes.  There is no "netif_set_speed()'
> function.  When a link status change occurs the kernel driver calls
> netif_carrier_on/off() and prints a message like this one.

I am not suggesting reflecting these into interface, I am just saying why do you
print them?

For example, "link->link_speed" this is coming as parameter to API, this API
does nothing with this value, why print is here?

I assume you are using "rte_eth_link" as parameter, instead of basic
"link_status" to make it extendible in the feature. If so please print those
other values when function extended, right now only link_status matters.

> 
>> <...>
>>
>>> @@ -148,9 +239,16 @@ test_kni_loop(__rte_unused void *arg)
>>>                       ret = -1;
>>>               if (system(IFCONFIG TEST_KNI_PORT" mtu 1400") == -1)
>>>                       ret = -1;
>>> +
>>> +             ret = kni_change_link();
>>> +             if (ret != 0) {
>>> +                     test_kni_processing_flag = 1;
>>> +                     return ret;
>>> +             }
>>
>> I thinks this is wrong place to call kni_change_link(), this is test_kni_loop()
>> created by test_kni_processing() that does packet processing tests.
>>
> 
> Well, no it's not "wrong".  The interface has to be up to change the
> link state, so this is a convenient place to do it.

Are we agree that this function in unit test is to test packet processing part
of KNI?
And for example please check following coming patch:
https://patches.dpdk.org/patch/44730/

I think you want to benefit from "system(IFCONFIG TEST_KNI_PORT" up")" since
interface needs to be up to set the link, but you can do same call, not have to
use that one.

> 
>> I think better to call directly from test_kni(), perhaps before
>> test_kni_processing() call?
> 
> Because then we'd have to add code to set the interface up and down
> again, and there really is no point.  This is as good a place as any.
> It does not affect the data transfer tests at all.

Doesn't affect the data transfer, agreed, but why confusing data transfer test
with link up/down calls? Why not have your function and set interface up and
down again?

> 
> -d
>