From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id C544A1B397 for ; Wed, 26 Sep 2018 18:42:56 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Sep 2018 09:42:55 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,306,1534834800"; d="scan'208";a="260443805" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.39]) ([10.237.221.39]) by orsmga005.jf.intel.com with ESMTP; 26 Sep 2018 09:42:54 -0700 To: Dan Gora Cc: dev@dpdk.org, Igor Ryzhov , Stephen Hemminger 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: Ferruh Yigit Openpgp: preference=signencrypt Message-ID: Date: Wed, 26 Sep 2018 17:42:54 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 16:42:57 -0000 On 9/26/2018 3:55 PM, Dan Gora wrote: > On Wed, Sep 26, 2018 at 10:59 AM, Ferruh Yigit 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 >> >> <...> >> >>> +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 >