[v3,4/4] ethdev: support MAC address as iterator filter
Checks
Commit Message
The MAC addresses of a port can be matched with devargs.
As the conflict between rte_ether.h and netinet/ether.h is not resolved,
the MAC parsing is done with a rte_cmdline function.
As a result, cmdline library becomes a dependency of ethdev.
Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
lib/Makefile | 1 +
lib/librte_cmdline/meson.build | 4 ++++
lib/librte_ethdev/Makefile | 2 +-
lib/librte_ethdev/meson.build | 2 +-
lib/librte_ethdev/rte_class_eth.c | 36 +++++++++++++++++++++++++++++++
lib/meson.build | 5 +++--
6 files changed, 46 insertions(+), 4 deletions(-)
Comments
On 10/22/18 4:15 PM, Thomas Monjalon wrote:
> The MAC addresses of a port can be matched with devargs.
>
> As the conflict between rte_ether.h and netinet/ether.h is not resolved,
> the MAC parsing is done with a rte_cmdline function.
> As a result, cmdline library becomes a dependency of ethdev.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
I'd like to share my thought about a new dependency.
Looking at cmdline I think that it is a bad and strange
dependency for kvargs. IMHO, even duplication of the
code to parse MAC address it less evil in this case.
May be it is possible to provide internal wrapper
which is implemented using ether_aton_r() and located
in a separate C file which does not include rte_ether.h etc?
Andrew.
22/10/2018 15:37, Andrew Rybchenko:
> On 10/22/18 4:15 PM, Thomas Monjalon wrote:
> > The MAC addresses of a port can be matched with devargs.
> >
> > As the conflict between rte_ether.h and netinet/ether.h is not resolved,
> > the MAC parsing is done with a rte_cmdline function.
> > As a result, cmdline library becomes a dependency of ethdev.
> >
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>
> I'd like to share my thought about a new dependency.
> Looking at cmdline I think that it is a bad and strange
> dependency for kvargs. IMHO, even duplication of the
> code to parse MAC address it less evil in this case.
cmdline is not a dependency for kvargs.
I have added it as a dependency for ethdev.
> May be it is possible to provide internal wrapper
> which is implemented using ether_aton_r() and located
> in a separate C file which does not include rte_ether.h etc?
I raised the issue in technical board and it has been decided to fix the
conflict with libc in the next release (with Olivier's help).
So Bruce and me decided to use cmdline function in the meantime.
On 10/22/18 5:02 PM, Thomas Monjalon wrote:
> 22/10/2018 15:37, Andrew Rybchenko:
>> On 10/22/18 4:15 PM, Thomas Monjalon wrote:
>>> The MAC addresses of a port can be matched with devargs.
>>>
>>> As the conflict between rte_ether.h and netinet/ether.h is not resolved,
>>> the MAC parsing is done with a rte_cmdline function.
>>> As a result, cmdline library becomes a dependency of ethdev.
>>>
>>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>> I'd like to share my thought about a new dependency.
>> Looking at cmdline I think that it is a bad and strange
>> dependency for kvargs. IMHO, even duplication of the
>> code to parse MAC address it less evil in this case.
> cmdline is not a dependency for kvargs.
> I have added it as a dependency for ethdev.
>
>> May be it is possible to provide internal wrapper
>> which is implemented using ether_aton_r() and located
>> in a separate C file which does not include rte_ether.h etc?
> I raised the issue in technical board and it has been decided to fix the
> conflict with libc in the next release (with Olivier's help).
> So Bruce and me decided to use cmdline function in the meantime.
OK, I see. Thanks for explanations.
On 10/22/18 4:15 PM, Thomas Monjalon wrote:
> The MAC addresses of a port can be matched with devargs.
>
> As the conflict between rte_ether.h and netinet/ether.h is not resolved,
> the MAC parsing is done with a rte_cmdline function.
> As a result, cmdline library becomes a dependency of ethdev.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Monday, October 22, 2018 3:02 PM
> To: Andrew Rybchenko <arybchenko@solarflare.com>
> Cc: dev@dpdk.org; gaetan.rivet@6wind.com; ophirmu@mellanox.com; Yigit, Ferruh <ferruh.yigit@intel.com>; olivier.matz@6wind.com;
> Horton, Remy <remy.horton@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>; olivier.matz@6wind.com
> Subject: Re: [dpdk-dev] [PATCH v3 4/4] ethdev: support MAC address as iterator filter
>
> 22/10/2018 15:37, Andrew Rybchenko:
> > On 10/22/18 4:15 PM, Thomas Monjalon wrote:
> > > The MAC addresses of a port can be matched with devargs.
> > >
> > > As the conflict between rte_ether.h and netinet/ether.h is not resolved,
> > > the MAC parsing is done with a rte_cmdline function.
> > > As a result, cmdline library becomes a dependency of ethdev.
> > >
> > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> >
> > I'd like to share my thought about a new dependency.
> > Looking at cmdline I think that it is a bad and strange
> > dependency for kvargs. IMHO, even duplication of the
> > code to parse MAC address it less evil in this case.
>
> cmdline is not a dependency for kvargs.
> I have added it as a dependency for ethdev.
>
> > May be it is possible to provide internal wrapper
> > which is implemented using ether_aton_r() and located
> > in a separate C file which does not include rte_ether.h etc?
>
> I raised the issue in technical board and it has been decided to fix the
> conflict with libc in the next release (with Olivier's help).
> So Bruce and me decided to use cmdline function in the meantime.
As I can see, cmdline_parse_etheraddr() uses
static struct ether_addr *
my_ether_aton(const char *a)
internally.
Why not to make it public, rename to rte_ethet_aton(),
and move into rte_net?
And use that one instead?
Later if/when we'll have name conflict with libc resolved,
It can become just a wrapper around ether_aton_r().
Konstantin
22/10/2018 23:24, Ananyev, Konstantin:
> From: Thomas Monjalon
> > 22/10/2018 15:37, Andrew Rybchenko:
> > > On 10/22/18 4:15 PM, Thomas Monjalon wrote:
> > > > The MAC addresses of a port can be matched with devargs.
> > > >
> > > > As the conflict between rte_ether.h and netinet/ether.h is not resolved,
> > > > the MAC parsing is done with a rte_cmdline function.
> > > > As a result, cmdline library becomes a dependency of ethdev.
> > > >
> > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > >
> > > I'd like to share my thought about a new dependency.
> > > Looking at cmdline I think that it is a bad and strange
> > > dependency for kvargs. IMHO, even duplication of the
> > > code to parse MAC address it less evil in this case.
> >
> > cmdline is not a dependency for kvargs.
> > I have added it as a dependency for ethdev.
> >
> > > May be it is possible to provide internal wrapper
> > > which is implemented using ether_aton_r() and located
> > > in a separate C file which does not include rte_ether.h etc?
> >
> > I raised the issue in technical board and it has been decided to fix the
> > conflict with libc in the next release (with Olivier's help).
> > So Bruce and me decided to use cmdline function in the meantime.
>
> As I can see, cmdline_parse_etheraddr() uses
> static struct ether_addr *
> my_ether_aton(const char *a)
> internally.
> Why not to make it public, rename to rte_ethet_aton(),
> and move into rte_net?
> And use that one instead?
> Later if/when we'll have name conflict with libc resolved,
> It can become just a wrapper around ether_aton_r().
> Konstantin
Several reasons:
- It would be one more (useless) wrapper
- cmdline_parse_etheraddr is already used in several places
- ether_aton_r and my_ether_aton may have a different behaviour
When the libc conflict will be solved, I prefer replacing uses of
cmdline_parse_etheraddr one by one.
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, October 23, 2018 8:21 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org; gaetan.rivet@6wind.com; ophirmu@mellanox.com; Yigit, Ferruh
> <ferruh.yigit@intel.com>; olivier.matz@6wind.com; Horton, Remy <remy.horton@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 4/4] ethdev: support MAC address as iterator filter
>
> 22/10/2018 23:24, Ananyev, Konstantin:
> > From: Thomas Monjalon
> > > 22/10/2018 15:37, Andrew Rybchenko:
> > > > On 10/22/18 4:15 PM, Thomas Monjalon wrote:
> > > > > The MAC addresses of a port can be matched with devargs.
> > > > >
> > > > > As the conflict between rte_ether.h and netinet/ether.h is not resolved,
> > > > > the MAC parsing is done with a rte_cmdline function.
> > > > > As a result, cmdline library becomes a dependency of ethdev.
> > > > >
> > > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > >
> > > > I'd like to share my thought about a new dependency.
> > > > Looking at cmdline I think that it is a bad and strange
> > > > dependency for kvargs. IMHO, even duplication of the
> > > > code to parse MAC address it less evil in this case.
> > >
> > > cmdline is not a dependency for kvargs.
> > > I have added it as a dependency for ethdev.
> > >
> > > > May be it is possible to provide internal wrapper
> > > > which is implemented using ether_aton_r() and located
> > > > in a separate C file which does not include rte_ether.h etc?
> > >
> > > I raised the issue in technical board and it has been decided to fix the
> > > conflict with libc in the next release (with Olivier's help).
> > > So Bruce and me decided to use cmdline function in the meantime.
> >
> > As I can see, cmdline_parse_etheraddr() uses
> > static struct ether_addr *
> > my_ether_aton(const char *a)
> > internally.
> > Why not to make it public, rename to rte_ethet_aton(),
> > and move into rte_net?
> > And use that one instead?
> > Later if/when we'll have name conflict with libc resolved,
> > It can become just a wrapper around ether_aton_r().
> > Konstantin
>
> Several reasons:
> - It would be one more (useless) wrapper
Well, it would be *when* will have libc naming conflict resolved.
Till that it would be pretty useful I think.
> - cmdline_parse_etheraddr is already used in several places
As I can see right now it is used just by bond PMD:
$ find lib drivers -type f | xargs grep -l cmdline_parse_etheraddr | grep -v librte_cmdline
drivers/net/bonding/rte_eth_bond_args.c
Again if that function is *really* used in several places to convert string to mac,
then I suppose it is an indication that rte_ether_aton() would be useful.
Without forcing cmdline dependency.
> - ether_aton_r and my_ether_aton may have a different behavior
Could you elaborate here?
What exactly would be different?
Both supposed to convert string to ether addr.
If our function can't do that properly, then I think it is a bug in our side.
If ether_aton_r() just supports extra formats(XXXX:XXXX:XXXX), then it
would be extension to current behavior.
>
> When the libc conflict will be solved, I prefer replacing uses of
> cmdline_parse_etheraddr one by one.
We can do the same with rte_ether_aton() too, if we really want to.
23/10/2018 10:33, Ananyev, Konstantin:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 22/10/2018 23:24, Ananyev, Konstantin:
> > > From: Thomas Monjalon
> > > > 22/10/2018 15:37, Andrew Rybchenko:
> > > > > On 10/22/18 4:15 PM, Thomas Monjalon wrote:
> > > > > > The MAC addresses of a port can be matched with devargs.
> > > > > >
> > > > > > As the conflict between rte_ether.h and netinet/ether.h is not resolved,
> > > > > > the MAC parsing is done with a rte_cmdline function.
> > > > > > As a result, cmdline library becomes a dependency of ethdev.
> > > > > >
> > > > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > > >
> > > > > I'd like to share my thought about a new dependency.
> > > > > Looking at cmdline I think that it is a bad and strange
> > > > > dependency for kvargs. IMHO, even duplication of the
> > > > > code to parse MAC address it less evil in this case.
> > > >
> > > > cmdline is not a dependency for kvargs.
> > > > I have added it as a dependency for ethdev.
> > > >
> > > > > May be it is possible to provide internal wrapper
> > > > > which is implemented using ether_aton_r() and located
> > > > > in a separate C file which does not include rte_ether.h etc?
> > > >
> > > > I raised the issue in technical board and it has been decided to fix the
> > > > conflict with libc in the next release (with Olivier's help).
> > > > So Bruce and me decided to use cmdline function in the meantime.
> > >
> > > As I can see, cmdline_parse_etheraddr() uses
> > > static struct ether_addr *
> > > my_ether_aton(const char *a)
> > > internally.
> > > Why not to make it public, rename to rte_ethet_aton(),
> > > and move into rte_net?
> > > And use that one instead?
> > > Later if/when we'll have name conflict with libc resolved,
> > > It can become just a wrapper around ether_aton_r().
> > > Konstantin
> >
> > Several reasons:
> > - It would be one more (useless) wrapper
>
> Well, it would be *when* will have libc naming conflict resolved.
> Till that it would be pretty useful I think.
It is planned to be fixed in the next release.
> > - cmdline_parse_etheraddr is already used in several places
>
> As I can see right now it is used just by bond PMD:
> $ find lib drivers -type f | xargs grep -l cmdline_parse_etheraddr | grep -v librte_cmdline
> drivers/net/bonding/rte_eth_bond_args.c
It is also used in examples:
examples/bond/main.c
examples/ethtool/ethtool-app/ethapp.c
examples/l3fwd/main.c
examples/performance-thread/l3fwd-thread/main.c
> Again if that function is *really* used in several places to convert string to mac,
> then I suppose it is an indication that rte_ether_aton() would be useful.
> Without forcing cmdline dependency.
I don't like wrappers and reinventing the wheel.
If we introduce this new wrapper, then we will have to deprecate it,
and break the API to remove it.
> > - ether_aton_r and my_ether_aton may have a different behavior
>
> Could you elaborate here?
> What exactly would be different?
The error path might be slightly different,
and...
> Both supposed to convert string to ether addr.
> If our function can't do that properly, then I think it is a bug in our side.
> If ether_aton_r() just supports extra formats(XXXX:XXXX:XXXX), then it
> would be extension to current behavior.
... yes there is this extension.
> > When the libc conflict will be solved, I prefer replacing uses of
> > cmdline_parse_etheraddr one by one.
>
> We can do the same with rte_ether_aton() too, if we really want to.
At the price of breaking the API again.
>
> 23/10/2018 10:33, Ananyev, Konstantin:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > 22/10/2018 23:24, Ananyev, Konstantin:
> > > > From: Thomas Monjalon
> > > > > 22/10/2018 15:37, Andrew Rybchenko:
> > > > > > On 10/22/18 4:15 PM, Thomas Monjalon wrote:
> > > > > > > The MAC addresses of a port can be matched with devargs.
> > > > > > >
> > > > > > > As the conflict between rte_ether.h and netinet/ether.h is not resolved,
> > > > > > > the MAC parsing is done with a rte_cmdline function.
> > > > > > > As a result, cmdline library becomes a dependency of ethdev.
> > > > > > >
> > > > > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > > > >
> > > > > > I'd like to share my thought about a new dependency.
> > > > > > Looking at cmdline I think that it is a bad and strange
> > > > > > dependency for kvargs. IMHO, even duplication of the
> > > > > > code to parse MAC address it less evil in this case.
> > > > >
> > > > > cmdline is not a dependency for kvargs.
> > > > > I have added it as a dependency for ethdev.
> > > > >
> > > > > > May be it is possible to provide internal wrapper
> > > > > > which is implemented using ether_aton_r() and located
> > > > > > in a separate C file which does not include rte_ether.h etc?
> > > > >
> > > > > I raised the issue in technical board and it has been decided to fix the
> > > > > conflict with libc in the next release (with Olivier's help).
> > > > > So Bruce and me decided to use cmdline function in the meantime.
> > > >
> > > > As I can see, cmdline_parse_etheraddr() uses
> > > > static struct ether_addr *
> > > > my_ether_aton(const char *a)
> > > > internally.
> > > > Why not to make it public, rename to rte_ethet_aton(),
> > > > and move into rte_net?
> > > > And use that one instead?
> > > > Later if/when we'll have name conflict with libc resolved,
> > > > It can become just a wrapper around ether_aton_r().
> > > > Konstantin
> > >
> > > Several reasons:
> > > - It would be one more (useless) wrapper
> >
> > Well, it would be *when* will have libc naming conflict resolved.
> > Till that it would be pretty useful I think.
>
> It is planned to be fixed in the next release.
>
> > > - cmdline_parse_etheraddr is already used in several places
> >
> > As I can see right now it is used just by bond PMD:
> > $ find lib drivers -type f | xargs grep -l cmdline_parse_etheraddr | grep -v librte_cmdline
> > drivers/net/bonding/rte_eth_bond_args.c
>
> It is also used in examples:
>
> examples/bond/main.c
> examples/ethtool/ethtool-app/ethapp.c
> examples/l3fwd/main.c
> examples/performance-thread/l3fwd-thread/main.c
>
> > Again if that function is *really* used in several places to convert string to mac,
> > then I suppose it is an indication that rte_ether_aton() would be useful.
> > Without forcing cmdline dependency.
>
> I don't like wrappers and reinventing the wheel.
> If we introduce this new wrapper, then we will have to deprecate it,
> and break the API to remove it.
>
> > > - ether_aton_r and my_ether_aton may have a different behavior
> >
> > Could you elaborate here?
> > What exactly would be different?
>
> The error path might be slightly different,
> and...
>
> > Both supposed to convert string to ether addr.
> > If our function can't do that properly, then I think it is a bug in our side.
> > If ether_aton_r() just supports extra formats(XXXX:XXXX:XXXX), then it
> > would be extension to current behavior.
>
> ... yes there is this extension.
>
> > > When the libc conflict will be solved, I prefer replacing uses of
> > > cmdline_parse_etheraddr one by one.
> >
> > We can do the same with rte_ether_aton() too, if we really want to.
>
> At the price of breaking the API again.
Probably you are right - extra dependency is a less evil here.
Konstantin
@@ -25,6 +25,7 @@ DIRS-$(CONFIG_RTE_LIBRTE_ETHER) += librte_ethdev
DEPDIRS-librte_ethdev := librte_net librte_eal librte_mempool librte_ring
DEPDIRS-librte_ethdev += librte_mbuf
DEPDIRS-librte_ethdev += librte_kvargs
+DEPDIRS-librte_ethdev += librte_cmdline
DIRS-$(CONFIG_RTE_LIBRTE_BBDEV) += librte_bbdev
DEPDIRS-librte_bbdev := librte_eal librte_mempool librte_mbuf
DIRS-$(CONFIG_RTE_LIBRTE_CRYPTODEV) += librte_cryptodev
@@ -1,6 +1,10 @@
# SPDX-License-Identifier: BSD-3-Clause
# Copyright(c) 2017 Intel Corporation
+# This library is processed before EAL
+includes = [global_inc]
+includes += include_directories('../librte_eal/common/include')
+
version = 2
sources = files('cmdline.c',
'cmdline_cirbuf.c',
@@ -12,7 +12,7 @@ CFLAGS += -DALLOW_EXPERIMENTAL_API
CFLAGS += -O3
CFLAGS += $(WERROR_FLAGS)
LDLIBS += -lrte_net -lrte_eal -lrte_mempool -lrte_ring
-LDLIBS += -lrte_mbuf -lrte_kvargs
+LDLIBS += -lrte_mbuf -lrte_kvargs -lrte_cmdline
EXPORT_MAP := rte_ethdev_version.map
@@ -26,4 +26,4 @@ headers = files('rte_ethdev.h',
'rte_tm.h',
'rte_tm_driver.h')
-deps += ['net', 'kvargs']
+deps += ['net', 'kvargs', 'cmdline']
@@ -4,6 +4,7 @@
#include <string.h>
+#include <cmdline_parse_etheraddr.h>
#include <rte_class.h>
#include <rte_compat.h>
#include <rte_errno.h>
@@ -16,11 +17,13 @@
#include "ethdev_private.h"
enum eth_params {
+ RTE_ETH_PARAM_MAC,
RTE_ETH_PARAM_REPRESENTOR,
RTE_ETH_PARAM_MAX,
};
static const char * const eth_params_keys[] = {
+ [RTE_ETH_PARAM_MAC] = "mac",
[RTE_ETH_PARAM_REPRESENTOR] = "representor",
[RTE_ETH_PARAM_MAX] = NULL,
};
@@ -36,6 +39,33 @@ struct eth_dev_match_arg {
.kvlist = (k), \
})
+static int
+eth_mac_cmp(const char *key __rte_unused,
+ const char *value, void *opaque)
+{
+ int ret;
+ struct ether_addr mac;
+ const struct rte_eth_dev_data *data = opaque;
+ struct rte_eth_dev_info dev_info;
+ uint32_t index;
+
+ /* Parse devargs MAC address. */
+ /*
+ * cannot use ether_aton_r(value, &mac)
+ * because of include conflict with rte_ether.h
+ */
+ ret = cmdline_parse_etheraddr(NULL, value, &mac, sizeof(mac));
+ if (ret < 0)
+ return -1; /* invalid devargs value */
+
+ /* Return 0 if devargs MAC is matching one of the device MACs. */
+ rte_eth_dev_info_get(data->port_id, &dev_info);
+ for (index = 0; index < dev_info.max_mac_addrs; index++)
+ if (is_same_ether_addr(&mac, &data->mac_addrs[index]))
+ return 0;
+ return -1; /* no match */
+}
+
static int
eth_representor_cmp(const char *key __rte_unused,
const char *value, void *opaque)
@@ -85,6 +115,12 @@ eth_dev_match(const struct rte_eth_dev *edev,
/* Empty string matches everything. */
return 0;
+ ret = rte_kvargs_process(kvlist,
+ eth_params_keys[RTE_ETH_PARAM_MAC],
+ eth_mac_cmp, edev->data);
+ if (ret != 0)
+ return -1;
+
ret = rte_kvargs_process(kvlist,
eth_params_keys[RTE_ETH_PARAM_REPRESENTOR],
eth_representor_cmp, edev->data);
@@ -9,13 +9,14 @@
# given as a dep, no need to mention ring. This is especially true for the
# core libs which are widely reused, so their deps are kept to a minimum.
libraries = [ 'compat', # just a header, used for versioning
- 'kvargs',
+ 'cmdline', # ethdev depends on cmdline for parsing functions
+ 'kvargs', # eal depends on kvargs
'eal', 'ring', 'mempool', 'mbuf', 'net', 'ethdev', 'pci', # core
'metrics', # bitrate/latency stats depends on this
'hash', # efd depends on this
'timer', # eventdev depends on this
'acl', 'bbdev', 'bitratestats', 'cfgfile',
- 'cmdline', 'compressdev', 'cryptodev',
+ 'compressdev', 'cryptodev',
'distributor', 'efd', 'eventdev',
'gro', 'gso', 'ip_frag', 'jobstats',
'kni', 'latencystats', 'lpm', 'member',