From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by dpdk.org (Postfix) with ESMTP id D24621B6CA for ; Wed, 10 Oct 2018 16:58:06 +0200 (CEST) Received: by mail-wr1-f66.google.com with SMTP id x12-v6so6093733wru.8 for ; Wed, 10 Oct 2018 07:58:06 -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=ZGhRii7C3FBBwPRe1Ma3UKn16DuuxvHTw5k9JPVaYJg=; b=RVRVnWeTQWOS/RD5DQ8mtX2LgUPYi5/Z+i35+BMtqHpq49FtkrRDOH94qbn4IqlJd6 EiYbsti/9aEQhnWgooUREr90TvfzOGv214i7GCmzFTg9x0bPGnU/+/pG2U2/1Jncj+M+ DiVot2UITxCX7tTfdwmU7XcDyScPPIBBlxLX8i3y9cEI2q2dhrfKaFJuMV0GLOiMCuKG +rw/XOOUCWiBkwUklFXyeyAMubuwVgdDh+s66z6w+eVwCZzhkE1N5yErTTkBnt2HU0sC lQb5cU5jIA882QJLl4gE9gdr298umgoHZaLuGRlTDAjXHvLQhUTQxsSnv1kU3QDqE8vV AnNg== X-Gm-Message-State: ABuFfogwLurHhe8spE3UGs140tCc28PhVdsRTtScaGYY0AbAabjX0SI5 sc9rHEe8ZCCo+ipeD76irEO2itdfvPUttMqUX2o= X-Google-Smtp-Source: ACcGV63lltQVqVb1EZkJl2mBnm01VQEkyukSTG3nr0yG2fNztrwpxo0SnAnaSmlwpylleYYMBO7cJsZ/XHPLIP4VNVM= X-Received: by 2002:adf:b519:: with SMTP id a25-v6mr22471465wrd.273.1539183486142; Wed, 10 Oct 2018 07:58:06 -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: From: Dan Gora Date: Wed, 10 Oct 2018 11:57:29 -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, 10 Oct 2018 14:58:07 -0000 On Wed, Oct 10, 2018 at 11:09 AM Ferruh Yigit wrote: > > 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. > > This API has nothing with if KNI interface has corresponding physical device or > not, all KNI users can use it. > I don't know what this means. > > 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. > > That is the think, you are not setting anything, you are just printing > "0Mbps/autoNeg/Full" and assuming/hoping the _if_ there is a corresponding > physical device app sends the values of that device to API for printing. ugh.. Again the API doesn't care if the values are "right". The application writer probably does though. > Overall I am not sure if there is a value to discuss this more, and I don't want > this relatively small issue to block the patch, I will comment more to latest > version. Agreed, I don't know how else I can express myself to get my point across. Can you at least comment on the rest of this email below that I spent a lot of time writing, trying to explain what I'm talking about? > >>>> 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