From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by dpdk.org (Postfix) with ESMTP id 74B201B1D7 for ; Wed, 26 Sep 2018 21:17:25 +0200 (CEST) Received: by mail-wr1-f67.google.com with SMTP id b11-v6so63140wru.9 for ; Wed, 26 Sep 2018 12:17:25 -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=JFVzRY8tuWrX+rKO/+rQJOCY1Gg70K2xd8Hroj6RidE=; b=Hc2GTL+sv5b47uAB9ZjH+n7Yf4K27qXfQfEv/u8UD65W8bMybroB/WYZn3VDVRRxHb DDH4v7mCUT6BFT8XNT+/Z6KJyUmPND2Xx9Wm43EWzgf81PM7jXvYXiWwPtFvvDNfVQoH +Hb/CrGrfqJtQFbV5JDD5ip03G6yAPkZzL4qmca7Ye6vRrUZHkP1DnB28byVnCglwDXb YVXGLo8Wq5o/Fg/d9TcDSbQoQfNPZGFnO/NqSLZE3hY11zGall0q2FEXB+JFZ7p81Syu 7ugHr+fhzqcpN6BXv6/B6AqLD/linIrz1WWZRKN5r2YndyGX+VPtrMNGrSCeVmg1eNqq SMZw== 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=JFVzRY8tuWrX+rKO/+rQJOCY1Gg70K2xd8Hroj6RidE=; b=G4ti1VYqloAh4UAtFQZZLVDJNk7fo5AwjjZKSoO8xtTtZDJRwcmcVvt6DckyexwMZT eN6A5JvqUHAB08SbZLKzDRg0J+quNQ2JNlznlsJAMXWH6byhZGzfvy+hwzNtmq5vAGe6 sTSJPXRBHyR8rVCL33R7HGlcWQj/qHPIclEe4fpDMyI5E7CLUzZPYtFC8hnctITAxhip dHZi3eUkbzBRxV0csWbXpPqUtWSDq9zHTsB6dqsP352rWkHbOVOljwO9dV5UUOnOiX0X gYXAYrXIn8LVXat3tWAHXfMHTOG3jBUuEKmY2IFeabUG8/JExtw3vEJWbasN9ZfGzyuZ ilFA== X-Gm-Message-State: ABuFfogoc5QLPjErn1gSuBH1qY0ByYfnNFauElm/wiApiNc2rC855md3 93yROZx4ohRXJ4ajHwbXc4J5Ah577Ir7iWhlnLk= X-Google-Smtp-Source: ACcGV62d+ei23WPPngnkxUcmgG73BdZtRETZGXS+s1LNBxAqhOjqc2MHOX2qDLUKPoTcq6sgaPM4WtGjR1q6knl6ROw= X-Received: by 2002:adf:f681:: with SMTP id v1-v6mr6135494wrp.201.1537989444830; Wed, 26 Sep 2018 12:17:24 -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 12:16:44 -0700 (PDT) In-Reply-To: <57ecd39f-70c8-d9b9-1c61-c665f17ec1c6@intel.com> References: <20180911232906.18352-1-dg@adax.com> <20180919195549.5585-1-dg@adax.com> <20180919195549.5585-4-dg@adax.com> <57ecd39f-70c8-d9b9-1c61-c665f17ec1c6@intel.com> From: Dan Gora Date: Wed, 26 Sep 2018 16:16:44 -0300 X-Google-Sender-Auth: 19hlHL61rvl4904rO8q0f1ss5E0 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 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 19:17:25 -0000 On Wed, Sep 26, 2018 at 11:00 AM, Ferruh Yigit wrote: > 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. > I'll just remove it.. It's unnecessary. I don't see why the user should have to explicitly type 'ip link set up dev ' external to the application, but that's the way it was before, so that's fine with me. >> >> - 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. Uh, no it didn't. It would wait for up to 9 seconds for the links to come up and print a message, but if they didn't it would happily just chug right along. > Why removing this logic? I think it is good to keep it as it is. Because it was kind of unnecessary in the first place. Why 9 seconds? Why not 10? Why not 60? None of the following logic in the program depends on the link being up. >> @@ -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 I'm not really inclined to add new command line options here. I'll remove the code to set the interface 'up' so the user will continue to have to type 'ip link set up dev ' externally the way they do now. I don't see any reason why checking the link status would require a command line option. It affects nothing in the datapath.