From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <gaetan.rivet@6wind.com>
Received: from mail-wm0-f66.google.com (mail-wm0-f66.google.com [74.125.82.66])
by dpdk.org (Postfix) with ESMTP id 4F9EF2BA3
for <dev@dpdk.org>; Fri, 31 Aug 2018 12:22:29 +0200 (CEST)
Received: by mail-wm0-f66.google.com with SMTP id j192-v6so4848632wmj.1
for <dev@dpdk.org>; Fri, 31 Aug 2018 03:22:29 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=6wind-com.20150623.gappssmtp.com; s=20150623;
h=date:from:to:cc:subject:message-id:references:mime-version
:content-disposition:content-transfer-encoding:in-reply-to
:user-agent; bh=XddZM4KKPt5YiVCEsUVV6FAOmm03qi1eycrs0R2qfMQ=;
b=Vx4SDerwWvpeFQfRw5aGk9KB58UFdytg1pErV9nt2Eqw9h4wQhmMtJnimgZgq5zZT+
r1pLKdscF9OH5KzjC/FOVptxNfXQKJSkOqnpgu8zIKkdv6/97CWVbxEFA5rXa/nq+5q8
D/p62vY2YWudcLpmQ9tJCWytHFDhKZ3ST9xXx2xF62DcoQ8pS4OrueU/5Jxo/Yyy5jua
6WaFXgsc/g2dWU+8fxENTrjJK2fsNttYQWPv6AFpn/k7HvUS4dGPMDejCHATVcJnez0G
2bD2pup7ZNcg/M2WiAdgze5ISm8hO+QrtFV69S0O1+pgPuo408jF1pXVwFl2LuswctVY
DOzw==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20161025;
h=x-gm-message-state:date:from:to:cc:subject:message-id:references
:mime-version:content-disposition:content-transfer-encoding
:in-reply-to:user-agent;
bh=XddZM4KKPt5YiVCEsUVV6FAOmm03qi1eycrs0R2qfMQ=;
b=FzsoG74M5f3Vx5jgup+tFUnfSGEe3FwWULyKHCZ/WdWNOeGq8hD/gNNxYwlPoMbVu+
58Mr47FLKWz8czzy+yam0KozBroFt3yx7GAW2EzZSfJfobRtumDz3Ms34Egbfk/9+J7f
EslN21WaNv0rIfFy0LFUDd8GP17fFljHuoiAjaTY76Ww9UR8kDvL6WEIq9pSHPdW4/iX
f1Mn3ynUWxG+ypQLZHNIiwozcpporB3cH9cQlGosV3lBmKSt2KOYSzSjnRGOjWybHhg+
x5kTvcMVVbQU7ZdheGWP6TJcmjGMudXD5XokgMujRRwQ0cUrTw41zr7OWRTqvLcgl3lu
UJ5A==
X-Gm-Message-State: APzg51COxkLq+R3/RuAbniC1rk+vns9Q3UFX7FG7SUQ2MCKspywKTim6
QfjPaXpylALnd0laZZt6Wj6uxQ==
X-Google-Smtp-Source: ANB0VdZ0u1n4rBQidtKrSQOExd9Pe95H2V+zIvTKdtvIhL3uRxIsMjiSQfQZTaplHGwIIVquBcM94Q==
X-Received: by 2002:a1c:91cd:: with SMTP id
t196-v6mr3550077wmd.100.1535710948733;
Fri, 31 Aug 2018 03:22:28 -0700 (PDT)
Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com.
[62.23.145.78])
by smtp.gmail.com with ESMTPSA id a9-v6sm4687019wmf.28.2018.08.31.03.22.27
(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);
Fri, 31 Aug 2018 03:22:27 -0700 (PDT)
Date: Fri, 31 Aug 2018 12:22:11 +0200
From: =?iso-8859-1?Q?Ga=EBtan?= Rivet <gaetan.rivet@6wind.com>
To: Andrew Rybchenko <arybchenko@solarflare.com>
Cc: dev@dpdk.org
Message-ID: <20180831102211.pazx7fd3rvikmohi@bidouze.vm.6wind.com>
References: <cover.1535633783.git.gaetan.rivet@6wind.com>
<e42237129b4191c629a711cf6081b1045bc83986.1535633784.git.gaetan.rivet@6wind.com>
<378e2116-4682-1ff8-8dba-1562bc1e2530@solarflare.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <378e2116-4682-1ff8-8dba-1562bc1e2530@solarflare.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Subject: Re: [dpdk-dev] [PATCH v1 05/13] ethdev: add private generic device
iterator
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: Fri, 31 Aug 2018 10:22:29 -0000
On Fri, Aug 31, 2018 at 01:09:34PM +0300, Andrew Rybchenko wrote:
> On 08/30/2018 04:41 PM, Gaetan Rivet wrote:
> > This iterator can be customized with a comparison function that will
> > trigger a stopping condition.
> >
> > It can be leveraged to write several different iterators that have
> > similar but non-identical purposes.
> >
> > It is private to librte_ethdev.
> >
> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > ---
> > lib/librte_ethdev/Makefile | 1 +
> > lib/librte_ethdev/eth_private.c | 31 +++++++++++++++++++++++++++++++
> > lib/librte_ethdev/eth_private.h | 26 ++++++++++++++++++++++++++
> > lib/librte_ethdev/meson.build | 3 ++-
> > 4 files changed, 60 insertions(+), 1 deletion(-)
> > create mode 100644 lib/librte_ethdev/eth_private.c
> > create mode 100644 lib/librte_ethdev/eth_private.h
> >
> > diff --git a/lib/librte_ethdev/Makefile b/lib/librte_ethdev/Makefile
> > index 0935a275e..3c1c92cb9 100644
> > --- a/lib/librte_ethdev/Makefile
> > +++ b/lib/librte_ethdev/Makefile
> > @@ -18,6 +18,7 @@ EXPORT_MAP := rte_ethdev_version.map
> > LIBABIVER := 10
> > +SRCS-y += eth_private.c
> > SRCS-y += rte_ethdev.c
> > SRCS-y += rte_flow.c
> > SRCS-y += rte_tm.c
> > diff --git a/lib/librte_ethdev/eth_private.c b/lib/librte_ethdev/eth_private.c
> > new file mode 100644
> > index 000000000..d565568a0
> > --- /dev/null
> > +++ b/lib/librte_ethdev/eth_private.c
>
> Just a nit
> I think it is better to name it ethdev_private.c since we already
> have ethdev_profile.[ch] and it is about ethdev, not Ethernet.
>
Agreed.
> > @@ -0,0 +1,31 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2018 Gaëtan Rivet
> > + */
> > +
> > +#include "rte_ethdev.h"
> > +#include "eth_private.h"
> > +
> > +struct rte_eth_dev *
> > +eth_find_device(const struct rte_eth_dev *start, rte_eth_cmp_t cmp,
> > + const void *data)
> > +{
> > + struct rte_eth_dev *edev;
> > + ptrdiff_t idx;
> > +
> > + /* Avoid Undefined Behaviour */
> > + if (start != NULL &&
> > + (start < &rte_eth_devices[0] ||
> > + start > &rte_eth_devices[RTE_MAX_ETHPORTS]))
> > + return NULL;
> > + if (start != NULL)
> > + idx = start - &rte_eth_devices[0] + 1;
> > + else
> > + idx = 0;
> > + for (; idx < RTE_MAX_ETHPORTS; idx++) {
> > + edev = &rte_eth_devices[idx];
>
> Shouldn't we limit it to valid ports only?
> If no, I think it would be useful to highlight it in the function
> description that it iterates over all devices including unused.
>
I'm undecided about it, I wanted eth_find_device to be and internal
operator in the most generic way, allowing ethdev layer to move from
manually iterating to using this, to introduce the probable future
evolution of not using an array of rte_eth_devices.
So I thought of this operator as something stateless, only looking at
currently available memory and letting the comparison select what is
useful. Maybe for example this operator would be used to find the next
unused device, etc.
My doubts is that this is kind of future-proofing the design, something
that I don't think is good practice.
So, if you prefer something that takes care of state, as far as devargs
parsing is concerned, I'm ok with that. Otherwise it can stay that way,
UNUSED devices are taken care of afterward.
> > + if (cmp(edev, data) == 0)
> > + return edev;
> > + }
> > + return NULL;
> > +}
> > +
> > diff --git a/lib/librte_ethdev/eth_private.h b/lib/librte_ethdev/eth_private.h
> > new file mode 100644
> > index 000000000..0f5c6d5c4
> > --- /dev/null
> > +++ b/lib/librte_ethdev/eth_private.h
>
> ethdev_private.h
sure.
>
> <...>
--
Gaëtan Rivet
6WIND