From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f66.google.com (mail-wm1-f66.google.com [209.85.128.66]) by dpdk.org (Postfix) with ESMTP id 4DC891B115 for ; Wed, 3 Oct 2018 21:08:03 +0200 (CEST) Received: by mail-wm1-f66.google.com with SMTP id 185-v6so6713255wmt.2 for ; Wed, 03 Oct 2018 12:08:03 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=nmek9inYPvA9RrcmluAqibcOKF16OtfpHDW2Q9ia/d8=; b=DDp9rrh6/b+BHPFansLeLa1vsD9Iwh+wgVm+dVqQCVCQ+4M5RfcxfXaQX/kJS8jCGG Y7yNNeKcD05Z2Ew0KaKkzyYqL5Ti1kZ1fOjhynnFWWbHIssFOhdxYruolm4C1Di5w2KT du178gHVhu27mfTKP7YwF5OOhENZPvc1umVT80DjzMDJ7Ls4xTV195quTG/QSRLVQKhY v35hS+zwXVE/UNbLrBu7tHzVZb5z500elowvQcnKi3ZNaIU0MJiDsBBKsY4zFF2GWXGR YwRIr01Cj+h/oYl7jq51aauWamGifZzxe2yjueexl+jEERNCo2Vnwuu6xxG1llM7j7fv TTEw== X-Gm-Message-State: ABuFfognPaWJsyj7ii+krRIYc9EZ8ms1MT9bq3lsvveSl0s5XT268VwK kri+w+9AhYUncNn94wkuxEI0UIMnZB4lTOS+aNbIfKf9 X-Google-Smtp-Source: ACcGV63AcQHtfyVOZTHcSvy+8/LwiIoxBObSXcCXwNLu8JhUrESZx7VKNwAJrECkCkOktcHhQ1pYfPeqhFky3b5Yq/o= X-Received: by 2002:a1c:e4c6:: with SMTP id b189-v6mr2423833wmh.114.1538593682628; Wed, 03 Oct 2018 12:08:02 -0700 (PDT) MIME-Version: 1.0 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> <61731242-db6c-0c5d-bcab-e82b45e324d7@intel.com> <846eed94-2ed7-7b89-5a3e-696ec3674a26@intel.com> <7811a241-524d-21fd-4852-b63e2bba7332@intel.com> In-Reply-To: <7811a241-524d-21fd-4852-b63e2bba7332@intel.com> From: Dan Gora Date: Wed, 3 Oct 2018 16:07:25 -0300 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, 03 Oct 2018 19:08:03 -0000 On Fri, Sep 28, 2018 at 5:03 AM Ferruh Yigit wrote: > > On 9/28/2018 12:51 AM, Dan Gora wrote: > > On Thu, Sep 27, 2018 at 8:44 PM, Ferruh Yigit wrote: > >>> Well, yes the link_status (link up, link down) _is_ applied to the KNI > >>> interface. When that occurs, most people want to know what the link > >>> speed is that the link came up at. > >> > >> +1 to this, people would like to know link speed of the interface. > >> Are you printing link speed of interface? You are printing whatever user pass to > >> API. > > > > There is no such thing as "link speed of the interface". The link > > speed is the speed of the underlying Ethernet link that the interface > > corresponds to. This is true for all other ethernet interfaces in the > > kernel. > > This is an API to set link status of KNI interface, KNI interface is a virtual > interface and no need to be backed by a physical interface at all. Yes, I understand that. > Only kni sample application uses it in a way to match a physical interface to a > KNI interface, but please check KNI PMD where you can have multiple KNI > interface without any physical device at all. Yes, I understand that.. As I pointed out previously, if there is no physical device which corresponds to this KNI interface, the application can: 1) Not use this API at all, just as they do now. 2) Use the API and set the state to linkup/0Mbps/autoNeg/Full. This is perfectly reasonable and no one would be confused by this. > >> I guess you trust to user to provide correct values there, but since only link > >> up & down matters, what prevents user to leave other fields, like speed, just > >> random values? > > > > Nothing. What prevents anyone from providing random values for > > anything? The point of the API was to make it super simple, just: > > > > rte_eth_link_get_nowait(portid, &link); > > rte_kni_update_link(p[portid]->kni[i], &link); > > You are only thinking about this use case. Normally the input to API should be > verified right, for this case there is no way to verify it and vales are not > used at all, it is just printed in API. In most applications it is useful to have a message printed which shows the *change* in link status as well as the speed that the link came up at. If you don't provide the link speed, etc information, then the API is not really useful. Yes the application writer can provide whatever they want for the link speed/duplex/autoneg, but so what? Their application will have a nonsensical log message. It's not DPDK's job to enforce sanity on everyone's application. And no, not every input to every API function is verified. There are lots of examples in the DPDK where this is not the case. The parameters should be verified to ensure that they do not cause the application to "break", not necessarily so that they make sense (whatever that would mean in this case). It was recommended that we change the rte_kni_update_link() API to only use the link status as an input and print the log messages outside the API in the application code. However think about what that would entail: You only want to log *changes* in the link state, not log a message when you set the same state. This means that either: 1) the application would have to store the previous carrier state. 2) rte_kni_update_link could return the previous value read from the /sys/.../carrier file. 3) the application could read the /sys/.../carrier file before calling rte_kni_update_link to read the previous carrier state. In case 1 you're obliging all users of this API to carry around this extra state for no real reason. The API can easily read the /sys/.../carrier file to see the previous state while it has it open, just as it does in this patch. In case 2, the application can easily detect when the state changes, but then you end up with a return value something like -1 for errors, 0 for previous_link == down, 1 for previous_link == up. I fail to see how this is a better API design that what has already been proposed, but it's at least somewhat useful, even if awkward. In the DPDK application, you get something like: rte_eth_link_get_nowait(portid, &link); prev_link = rte_kni_update_link(p[portid]->kni[i], link.link_status); if (prev_link >= 0) { if (prev_link == 0 && link.link_status == 1) printf("LINKUP: speed %d duplex %s autoneg %s\n", ....) else if (prev_link == 1 && link.link_status == 0) printf("LINKDOWN:\n"); } else { printf("Error: something went wrong...\n"); } I don't really see how this is better than: rte_eth_link_get_nowait(portid, &link); rte_kni_update_link(p[portid]->kni[i], &link); but that's my opinion... In case 3, you might as well not even have this API since you're duplicating half of the same code. I would be willing to implement case 2, however I still think that it is not as good a design and will unnecessarily complicate application code compared to the current patch here. Cases 1 and 3 are non-starters for me. Our application needs log messages only when the link *changes* state. These log messages need to include the link speed/duplex/autoneg info. I think that most, if not all, applications would want that too. It's how every other physical ethernet driver in the linux kernel works. DPDK applications which use only KNI virtual interfaces without a physical interface are under no obligation to use this API function. thanks dan