From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f67.google.com (mail-wm1-f67.google.com [209.85.128.67]) by dpdk.org (Postfix) with ESMTP id 4A4C01B4A1 for ; Wed, 26 Sep 2018 20:56:54 +0200 (CEST) Received: by mail-wm1-f67.google.com with SMTP id 206-v6so3361667wmb.5 for ; Wed, 26 Sep 2018 11:56:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=FRlyrodHiTiN1XNhFrBKMURgBQT2y8yn69mzrYhpXOw=; b=Y1E7l9hS35/JicPcy6i8gKRUHKFcvJuX9kvXaOGmIOb8uAJqcNDEXCTRzvzMCzAidX sQb+IUrBlsCUvIqz56QGVUyCKV/DXDo0mQ4ZYsaybx0MyXS3SD1mfAlAf8XiwtYU1Uab dU/pFWKzwoEnNBXOu0F2pIFUlZrSjM9kYTfiAgOVArwgnCpW0uAphDdfQ/LVLjr660Y3 ee7Gl0jTVCLvcWT/xFhW6CgDOEGws6QP9cWJPIEwHGi7cJ7KmjCt5hg8FWW5ef2dJgUw CJBESgPB+uJP6WUovUUSmdKd6MABpT7qAzAD806HGcBikkTLxuTi+dyDbddF+srURw8U ATlA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=FRlyrodHiTiN1XNhFrBKMURgBQT2y8yn69mzrYhpXOw=; b=Trn0G2kWdxvsNJl/ZGtzVlt2V7Ibx3PIt/rk0EFHGeadq7oG4qdP6RWWQuYneFIot7 4EAkUsYgdElY/AFIGJ6j4aWZGrf9AjxpmFrfKFqb7/Zz/vhVw3ZcWMdPQgQsZuC6X3yc VePkIIr0ZOsdD75LXAaFdaYyBi7uloy3lB4jcb+KZl4ccH8NUhULKOGCzxyU1yE1q1wM 8WERUFLqKFvhBc3T5n511fCsZb39IuJH2V0le2aehQq/dmYJ9DhimvA7AkwVRYOpwaHZ 42Pvt1aseiCgspPFMJt9QApoSGYDdzgtknFM0gZFXzAFIdvTcsSzrY0Myq4NU8hOwIjN nLKQ== X-Gm-Message-State: ABuFfoiPZUIiRHpl5tuHziUm8YnD+UmuEacp8W2BUGYMaSebH3TcLwQG 8Jey//G/C7zRO3vSZWVdOoAJWqEXa/QOsisGKhI= X-Google-Smtp-Source: ACcGV63f85Aj2qy1Nk8kuze64Bt1tVV5eOBgejRWHqxPti4MBihljfuk3117NZhKaY5qeYg7p1fMh3Lae6yEWs16c9w= X-Received: by 2002:a1c:1a48:: with SMTP id a69-v6mr5333981wma.43.1537988213580; Wed, 26 Sep 2018 11:56:53 -0700 (PDT) MIME-Version: 1.0 Sender: dan.gora@gmail.com Received: by 2002:adf:fbc1:0:0:0:0:0 with HTTP; Wed, 26 Sep 2018 11:56:13 -0700 (PDT) In-Reply-To: References: <20180911232906.18352-1-dg@adax.com> <20180919195549.5585-1-dg@adax.com> <20180919195549.5585-2-dg@adax.com> <671135e5-a666-4254-c5c6-672c3863146b@intel.com> From: Dan Gora Date: Wed, 26 Sep 2018 15:56:13 -0300 X-Google-Sender-Auth: bPvMCyGvFOScRpPOAaX_ncP3o7Q Message-ID: To: Ferruh Yigit Cc: dev@dpdk.org, Igor Ryzhov , Stephen Hemminger Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v2 1/5] kni: add API to set link status on kernel interface X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 26 Sep 2018 18:56:55 -0000 On Wed, Sep 26, 2018 at 1:42 PM, Ferruh Yigit wrote: >> 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? Because the information is useful and because every other Ethernet driver does the same thing when the link status changes. See the Linux kernel: ixgbe_watchdog_link_is_up(): drivers/net/ethernet/intel/ixgbe/ixgbe_main.c print_link_info(): drivers/net/ethernet/cavium/liquidio/lio_main.c bnx2_report_link(): drivers/net/ethernet/broadcom/bnx2.c and dozens of other examples. Imagine you plug your 1G ethernet cable into a 10/100Mbps hub. Yes the link will be up, but it's the wrong speed. Imagine you had accidentally forced your 1G connection to 10Mbps fixed. It will only come up at 10Mbps. Wouldn't you like to know that autoNeg was disabled? > For example, "link->link_speed" this is coming as parameter to API, this API > does nothing with this value, why print is here? Because wouldn't you like to know if your link has connected at the correct speed? I would. > 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. No, all that information matters. If you have autoNeg disabled or are connecting to a broken piece of equipment, the link can come up but be at the wrong speed. There is not much to extend this function with. The only action we can take, other than to print the information, is to write to the /sys file to have the KNI kernel module call netif_carrier_on/off() for us. >>>> @@ -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. ok, I'll fix this. Should I rebase the changes on top of this patch (https://patches.dpdk.org/patch/44730/)? Is that patch going to get accepted soon? >> 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? ok. I was just trying to minimize the number of changes. I'll send a new patch.