From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 4FE271B14C for ; Wed, 26 Sep 2018 16:00:51 +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 fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Sep 2018 07:00:50 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,306,1534834800"; d="scan'208";a="260406987" 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 07:00:48 -0700 To: Dan Gora , dev@dpdk.org Cc: Igor Ryzhov , Stephen Hemminger References: <20180911232906.18352-1-dg@adax.com> <20180919195549.5585-1-dg@adax.com> <20180919195549.5585-4-dg@adax.com> From: Ferruh Yigit Openpgp: preference=signencrypt Message-ID: <57ecd39f-70c8-d9b9-1c61-c665f17ec1c6@intel.com> Date: Wed, 26 Sep 2018 15:00:48 +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: <20180919195549.5585-4-dg@adax.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2 3/5] examples/kni: monitor and update link status continually 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 14:00:51 -0000 On 9/19/2018 8:55 PM, Dan Gora wrote: > Update KNI example to continuously monitor the Ethernet link status of > the physical link and update the carrier status of the corresponding > interfaces with rte_kni_update_link(). > > Signed-off-by: Dan Gora Also this patch sets kni interfaces "up", please add this information to commit log. <...> > + RTE_ETH_FOREACH_DEV(portid) { > + for (i = 0; i < p[portid]->nb_kni; i++) { > + name = rte_kni_get_name(p[portid]->kni[i]); > + snprintf(cmd, sizeof(cmd), > + "/sbin/ifconfig %s up", name); > + RTE_LOG(INFO, APP, > + "Marking interface %s 'up'\n", name); > + if (system(cmd) == -1) > + RTE_LOG(ERR, APP, > + "Error: Failed to mark interface %s 'up'\n", > + name); > + } > + } This part can be separated thread, overall already done only once, and put into its own function. And what about making this optional with command line argument. > > - printf("\nChecking link status\n"); > - fflush(stdout); > - for (count = 0; count <= MAX_CHECK_TIME; count++) { > - all_ports_up = 1; > + while (kni_running) { > + rte_delay_ms(CHECK_INTERVAL); > RTE_ETH_FOREACH_DEV(portid) { > - if ((port_mask & (1 << portid)) == 0) > + if ((ports_mask & (1 << portid)) == 0) > continue; > memset(&link, 0, sizeof(link)); > rte_eth_link_get_nowait(portid, &link); > - /* print link status if flag set */ > - if (print_flag == 1) { > - if (link.link_status) > - printf( > - "Port%d Link Up - speed %uMbps - %s\n", > - portid, link.link_speed, > - (link.link_duplex == ETH_LINK_FULL_DUPLEX) ? > - ("full-duplex") : ("half-duplex\n")); > - else > - printf("Port %d Link Down\n", portid); > - continue; > - } > - /* clear all_ports_up flag if any link down */ > - if (link.link_status == ETH_LINK_DOWN) { > - all_ports_up = 0; > - break; > - } > - } > - /* after finally printing all link status, get out */ > - if (print_flag == 1) > - break; > - > - if (all_ports_up == 0) { > - printf("."); > - fflush(stdout); > - rte_delay_ms(CHECK_INTERVAL); > - } Previous logic makes sure all physical interfaces are up before packet processing is started. Why removing this logic? I think it is good to keep it as it is. <...> > @@ -947,7 +938,14 @@ main(int argc, char** argv) > > kni_alloc(port); > } > - check_all_ports_link_status(ports_mask); > + > + kni_running = 1; > + ret = rte_ctrl_thread_create(&kni_link_tid, > + "KNI link status check", NULL, > + check_all_ports_link_status, NULL); This thread is to reflect physical port link status to KNI interface, this can be useful but not sure to have it as default. Why not introduce this ability, and creating thread, as an optional feature enabled by command line option? So overall makes two commandline option, - one to set kni interfaces up by application - one to create a thread to check and reflect physical port link status to KNI interface