From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dan.gora@gmail.com>
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 <dev@dpdk.org>; Wed, 10 Oct 2018 16:58:06 +0200 (CEST)
Received: by mail-wr1-f66.google.com with SMTP id x12-v6so6093733wru.8
for <dev@dpdk.org>; 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>
<CAGyogRZx6THZuRreTLEBiYjZ8V_sbZk3HZywSprUU0PM5Wy7QA@mail.gmail.com>
<e8ba3242-2aea-87bf-3dd2-c1522285620e@intel.com>
<CAGyogRa5rF5B_0=U68Dj1A1cyQevO+Qeww-9zAh86ekce+n64A@mail.gmail.com>
<61731242-db6c-0c5d-bcab-e82b45e324d7@intel.com>
<CAGyogRZcZPBwj8JcE+--RHqx2SY-sZc0W1M26bSQkBVjyxZ_Mg@mail.gmail.com>
<d76bffde-506c-49b6-8db6-cfa79ad983d3@intel.com>
<CAGyogRaO95E=KN-J=oFroXVcPeZrAqOY4T+yDPWcGiHCZMpM0Q@mail.gmail.com>
<846eed94-2ed7-7b89-5a3e-696ec3674a26@intel.com>
<CAGyogRaNVQqgZLVsAHjH0Hwma6_Y+16CdjLpAkBg+Me3bL7xqw@mail.gmail.com>
<7811a241-524d-21fd-4852-b63e2bba7332@intel.com>
<CAGyogRZJmAWGXX0kXk+k69rbDbRqEJgS0H_Vw8h43-qFQ_ptEQ@mail.gmail.com>
<d0c77251-04d7-29e7-53ef-92ef2ce0f656@intel.com>
In-Reply-To: <d0c77251-04d7-29e7-53ef-92ef2ce0f656@intel.com>
From: Dan Gora <dg@adax.com>
Date: Wed, 10 Oct 2018 11:57:29 -0300
Message-ID: <CAGyogRbeTOVHintCHeadAQcveDUgH_WvFJXhHUskB-XmZPtZcw@mail.gmail.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>
Cc: dev@dpdk.org, Igor Ryzhov <iryzhov@nfware.com>,
Stephen Hemminger <stephen@networkplumber.org>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Wed, 10 Oct 2018 14:58:07 -0000
On Wed, Oct 10, 2018 at 11:09 AM Ferruh Yigit <ferruh.yigit@intel.com> 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