[dpdk-dev,v3,3/8] net/failsafe: support probed sub-devices getting
Checks
Commit Message
Previous fail-safe code didn't support getting probed sub-devices and
failed when it tried to probe them.
Skip fail-safe sub-device probing when it already was probed.
Signed-off-by: Matan Azrad <matan@mellanox.com>
Cc: Gaetan Rivet <gaetan.rivet@6wind.com>
---
doc/guides/nics/fail_safe.rst | 5 ++++
drivers/net/failsafe/failsafe_eal.c | 60 ++++++++++++++++++++++++-------------
2 files changed, 45 insertions(+), 20 deletions(-)
Comments
Hi Matan,
I'n not fond of the commit title, how about:
[PATCH v3 3/8] net/failsafe: add probed etherdev capture
?
On Tue, Jan 09, 2018 at 02:47:28PM +0000, Matan Azrad wrote:
> Previous fail-safe code didn't support getting probed sub-devices and
> failed when it tried to probe them.
>
> Skip fail-safe sub-device probing when it already was probed.
>
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> Cc: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
> doc/guides/nics/fail_safe.rst | 5 ++++
> drivers/net/failsafe/failsafe_eal.c | 60 ++++++++++++++++++++++++-------------
> 2 files changed, 45 insertions(+), 20 deletions(-)
>
> diff --git a/doc/guides/nics/fail_safe.rst b/doc/guides/nics/fail_safe.rst
> index 5b1b47e..b89e53b 100644
> --- a/doc/guides/nics/fail_safe.rst
> +++ b/doc/guides/nics/fail_safe.rst
> @@ -115,6 +115,11 @@ Fail-safe command line parameters
> order to take only the last line into account (unlike ``exec()``) at every
> probe attempt.
>
> +.. note::
> +
> + In case of whitelist sub-device probed by EAL, fail-safe PMD will take the device
> + as is, which means that EAL device options are taken in this case.
> +
> - **mac** parameter [MAC address]
>
> This parameter allows the user to set a default MAC address to the fail-safe
> diff --git a/drivers/net/failsafe/failsafe_eal.c b/drivers/net/failsafe/failsafe_eal.c
> index 19d26f5..7bc7453 100644
> --- a/drivers/net/failsafe/failsafe_eal.c
> +++ b/drivers/net/failsafe/failsafe_eal.c
> @@ -36,39 +36,59 @@
> #include "failsafe_private.h"
>
> static int
> +fs_get_port_by_device_name(const char *name, uint16_t *port_id)
The naming convention for the failsafe driver is
namespace_object_sub-object_action()
With an ordering of objects by their scope (std, rte, failsafe, file).
Also, "get" as an action is not descriptive enough.
static int
fs_ethdev_capture(const char *name, uint16_t *port_id);
> +{
> + uint16_t pid;
> + size_t len;
> +
> + if (name == NULL) {
> + DEBUG("Null pointer is specified\n");
> + return -EINVAL;
> + }
> + len = strlen(name);
> + RTE_ETH_FOREACH_DEV(pid) {
> + if (!strncmp(name, rte_eth_devices[pid].device->name, len)) {
> + *port_id = pid;
> + return 0;
> + }
> + }
> + return -ENODEV;
> +}
> +
> +static int
> fs_bus_init(struct rte_eth_dev *dev)
> {
> struct sub_device *sdev;
> struct rte_devargs *da;
> uint8_t i;
> - uint16_t j;
> + uint16_t pid;
> int ret;
>
> FOREACH_SUBDEV(sdev, i, dev) {
> if (sdev->state != DEV_PARSED)
> continue;
> da = &sdev->devargs;
> - ret = rte_eal_hotplug_add(da->bus->name,
> - da->name,
> - da->args);
> - if (ret) {
> - ERROR("sub_device %d probe failed %s%s%s", i,
> - rte_errno ? "(" : "",
> - rte_errno ? strerror(rte_errno) : "",
> - rte_errno ? ")" : "");
> - continue;
> - }
> - RTE_ETH_FOREACH_DEV(j) {
> - if (strcmp(rte_eth_devices[j].device->name,
> - da->name) == 0) {
> - ETH(sdev) = &rte_eth_devices[j];
> - break;
> + if (fs_get_port_by_device_name(da->name, &pid) != 0) {
> + ret = rte_eal_hotplug_add(da->bus->name,
> + da->name,
> + da->args);
> + if (ret) {
> + ERROR("sub_device %d probe failed %s%s%s", i,
> + rte_errno ? "(" : "",
> + rte_errno ? strerror(rte_errno) : "",
> + rte_errno ? ")" : "");
> + continue;
> }
> + if (fs_get_port_by_device_name(da->name, &pid) != 0) {
> + ERROR("sub_device %d init went wrong", i);
> + return -ENODEV;
> + }
> + } else {
> + /* Take control of device probed by EAL options. */
> + DEBUG("Taking control of a probed sub device"
> + " %d named %s", i, da->name);
In this case, the devargs of the probed device must be copied within the
sub-device definition and removed from the EAL using the proper
rte_devargs API.
Note that there is no rte_devargs copy function. You can use
rte_devargs_parse instead, "parsing" again the original devargs into the
sub-device one. It is necessary for complying with internal rte_devargs
requirements (da->args being malloc-ed, at the moment, but may evolve).
The rte_eal_devargs_parse function is not easy enough to use right now,
you will have to build a devargs string (using snprintf) and submit it.
I proposed a change this release for it but it will not make it for
18.02, that would have simplified your implementation.
Hi Gaetan
From: Gaëtan Rivet, Tuesday, January 16, 2018 1:09 PM
> Hi Matan,
>
> I'n not fond of the commit title, how about:
>
> [PATCH v3 3/8] net/failsafe: add probed etherdev capture
>
> ?
>
OK, no problem.
> On Tue, Jan 09, 2018 at 02:47:28PM +0000, Matan Azrad wrote:
> > Previous fail-safe code didn't support getting probed sub-devices and
> > failed when it tried to probe them.
> >
> > Skip fail-safe sub-device probing when it already was probed.
> >
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > Cc: Gaetan Rivet <gaetan.rivet@6wind.com>
> > ---
> > doc/guides/nics/fail_safe.rst | 5 ++++
> > drivers/net/failsafe/failsafe_eal.c | 60
> > ++++++++++++++++++++++++-------------
> > 2 files changed, 45 insertions(+), 20 deletions(-)
> >
> > diff --git a/doc/guides/nics/fail_safe.rst
> > b/doc/guides/nics/fail_safe.rst index 5b1b47e..b89e53b 100644
> > --- a/doc/guides/nics/fail_safe.rst
> > +++ b/doc/guides/nics/fail_safe.rst
> > @@ -115,6 +115,11 @@ Fail-safe command line parameters
> > order to take only the last line into account (unlike ``exec()``) at every
> > probe attempt.
> >
> > +.. note::
> > +
> > + In case of whitelist sub-device probed by EAL, fail-safe PMD will take the
> device
> > + as is, which means that EAL device options are taken in this case.
> > +
> > - **mac** parameter [MAC address]
> >
> > This parameter allows the user to set a default MAC address to the
> > fail-safe diff --git a/drivers/net/failsafe/failsafe_eal.c
> > b/drivers/net/failsafe/failsafe_eal.c
> > index 19d26f5..7bc7453 100644
> > --- a/drivers/net/failsafe/failsafe_eal.c
> > +++ b/drivers/net/failsafe/failsafe_eal.c
> > @@ -36,39 +36,59 @@
> > #include "failsafe_private.h"
> >
> > static int
> > +fs_get_port_by_device_name(const char *name, uint16_t *port_id)
>
> The naming convention for the failsafe driver is
>
> namespace_object_sub-object_action()
>
OK.
> With an ordering of objects by their scope (std, rte, failsafe, file).
> Also, "get" as an action is not descriptive enough.
>
Isn't "get by device name" descriptive?
> static int
> fs_ethdev_capture(const char *name, uint16_t *port_id);
>
You miss here the main reason why we need this function instead of using rte_eth_dev_get_port_by_name.
The reason we need this function is because we want to find the device by the device name and not ethdev name.
What's about fs_port_capture_by_device_name?
Maybe comparing it to device->devargs->name is better, What do you think?
> > +{
> > + uint16_t pid;
> > + size_t len;
> > +
> > + if (name == NULL) {
> > + DEBUG("Null pointer is specified\n");
> > + return -EINVAL;
> > + }
> > + len = strlen(name);
> > + RTE_ETH_FOREACH_DEV(pid) {
> > + if (!strncmp(name, rte_eth_devices[pid].device->name,
> len)) {
> > + *port_id = pid;
> > + return 0;
> > + }
> > + }
> > + return -ENODEV;
> > +}
> > +
> > +static int
> > fs_bus_init(struct rte_eth_dev *dev)
> > {
> > struct sub_device *sdev;
> > struct rte_devargs *da;
> > uint8_t i;
> > - uint16_t j;
> > + uint16_t pid;
> > int ret;
> >
> > FOREACH_SUBDEV(sdev, i, dev) {
> > if (sdev->state != DEV_PARSED)
> > continue;
> > da = &sdev->devargs;
> > - ret = rte_eal_hotplug_add(da->bus->name,
> > - da->name,
> > - da->args);
> > - if (ret) {
> > - ERROR("sub_device %d probe failed %s%s%s", i,
> > - rte_errno ? "(" : "",
> > - rte_errno ? strerror(rte_errno) : "",
> > - rte_errno ? ")" : "");
> > - continue;
> > - }
> > - RTE_ETH_FOREACH_DEV(j) {
> > - if (strcmp(rte_eth_devices[j].device->name,
> > - da->name) == 0) {
> > - ETH(sdev) = &rte_eth_devices[j];
> > - break;
> > + if (fs_get_port_by_device_name(da->name, &pid) != 0) {
> > + ret = rte_eal_hotplug_add(da->bus->name,
> > + da->name,
> > + da->args);
> > + if (ret) {
> > + ERROR("sub_device %d probe failed
> %s%s%s", i,
> > + rte_errno ? "(" : "",
> > + rte_errno ? strerror(rte_errno) : "",
> > + rte_errno ? ")" : "");
> > + continue;
> > }
> > + if (fs_get_port_by_device_name(da->name, &pid)
> != 0) {
> > + ERROR("sub_device %d init went wrong", i);
> > + return -ENODEV;
> > + }
> > + } else {
> > + /* Take control of device probed by EAL options. */
> > + DEBUG("Taking control of a probed sub device"
> > + " %d named %s", i, da->name);
>
> In this case, the devargs of the probed device must be copied within the sub-
> device definition and removed from the EAL using the proper rte_devargs
> API.
>
> Note that there is no rte_devargs copy function. You can use
> rte_devargs_parse instead, "parsing" again the original devargs into the sub-
> device one. It is necessary for complying with internal rte_devargs
> requirements (da->args being malloc-ed, at the moment, but may evolve).
>
> The rte_eal_devargs_parse function is not easy enough to use right now,
> you will have to build a devargs string (using snprintf) and submit it.
> I proposed a change this release for it but it will not make it for 18.02, that
> would have simplified your implementation.
>
Got you. You right we need to remove the created devargs in fail-safe parse level.
What do you think about checking it in the parse level and avoid the new devargs creation?
Also to do the copy in parse level(same method as we are doing in probe level)?
> --
> Gaëtan Rivet
> 6WIND
On Tue, Jan 16, 2018 at 12:27:57PM +0000, Matan Azrad wrote:
> Hi Gaetan
>
> From: Gaëtan Rivet, Tuesday, January 16, 2018 1:09 PM
> > Hi Matan,
> >
> > I'n not fond of the commit title, how about:
> >
> > [PATCH v3 3/8] net/failsafe: add probed etherdev capture
> >
> > ?
> >
> OK, no problem.
>
> > On Tue, Jan 09, 2018 at 02:47:28PM +0000, Matan Azrad wrote:
> > > Previous fail-safe code didn't support getting probed sub-devices and
> > > failed when it tried to probe them.
> > >
> > > Skip fail-safe sub-device probing when it already was probed.
> > >
> > > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > > Cc: Gaetan Rivet <gaetan.rivet@6wind.com>
> > > ---
> > > doc/guides/nics/fail_safe.rst | 5 ++++
> > > drivers/net/failsafe/failsafe_eal.c | 60
> > > ++++++++++++++++++++++++-------------
> > > 2 files changed, 45 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/doc/guides/nics/fail_safe.rst
> > > b/doc/guides/nics/fail_safe.rst index 5b1b47e..b89e53b 100644
> > > --- a/doc/guides/nics/fail_safe.rst
> > > +++ b/doc/guides/nics/fail_safe.rst
> > > @@ -115,6 +115,11 @@ Fail-safe command line parameters
> > > order to take only the last line into account (unlike ``exec()``) at every
> > > probe attempt.
> > >
> > > +.. note::
> > > +
> > > + In case of whitelist sub-device probed by EAL, fail-safe PMD will take the
> > device
> > > + as is, which means that EAL device options are taken in this case.
> > > +
> > > - **mac** parameter [MAC address]
> > >
> > > This parameter allows the user to set a default MAC address to the
> > > fail-safe diff --git a/drivers/net/failsafe/failsafe_eal.c
> > > b/drivers/net/failsafe/failsafe_eal.c
> > > index 19d26f5..7bc7453 100644
> > > --- a/drivers/net/failsafe/failsafe_eal.c
> > > +++ b/drivers/net/failsafe/failsafe_eal.c
> > > @@ -36,39 +36,59 @@
> > > #include "failsafe_private.h"
> > >
> > > static int
> > > +fs_get_port_by_device_name(const char *name, uint16_t *port_id)
> >
> > The naming convention for the failsafe driver is
> >
> > namespace_object_sub-object_action()
> >
> OK.
> > With an ordering of objects by their scope (std, rte, failsafe, file).
> > Also, "get" as an action is not descriptive enough.
> >
> Isn't "get by device name" descriptive?
The endgame is capturing a device that we know we are interested in.
The device name being used for matching is an implementation detail,
which should be abstracted by using a sub-function.
Putting this in the name defeat the reason for using another function.
> > static int
> > fs_ethdev_capture(const char *name, uint16_t *port_id);
> >
> You miss here the main reason why we need this function instead of using rte_eth_dev_get_port_by_name.
> The reason we need this function is because we want to find the device by the device name and not ethdev name.
> What's about fs_port_capture_by_device_name?
You are getting a port_id that is only valid for the rte_eth_devices
array, by using the ethdev iterator. You are only looking for an ethdev.
So it doesn't really matter whether you are using the ethdev name or the
device name, in the end you are capturing an ethdev
--> fs_ethdev_capture seems good for me.
Now, I guess you will say that the user would need to know that they
have to provide a device name that would be written in device->name. The
issue here is that you have a leaky abstraction for your function,
forcing this kind of consideration on your function user.
So I'd go further and will ask you to change the `const char *name` to a
`const rte_devargs *da` in the parameters.
> Maybe comparing it to device->devargs->name is better, What do you think?
>
You are touching at a pretty contentious subject here :) .
Identifying devices is not currently a well-defined function in DPDK.
Some ports (actually, only one model: ConnectX-3) will have several
ports using the same PCI slot. But even ignoring this glaring problem...
As it is, the device->name for PCI will match the name given as a
devargs, so functionally this should not change anything.
Furthermore, you will have devices probed without any devargs. The
fail-safe would thus be unable to capture non-blacklisted devices when
the PCI bus is in blacklist mode.
These not-blacklisted devices actually will have a full-PCI name (DomBDF
format), so a simple match with the one passed in your fail-safe devargs
will fail, ex:
# A physical port exists at 0000:00:02.0
testpmd --vdev="net_failsafe,dev(00:02.0)" -- -i
Would fail to capture the device 0000:00:02.0, as this is the name that
the PCI bus would give to this device, in the absence of a user-given
name.
In 18.05, or 18.08 there should be an EAL function that would be able to
identify a device given a specific ID string (very close to an
rte_devargs). Currently, this API does not exist.
You can hack your way around this for the moment, IF you really, really
want: parse your devargs, get the bus, use the bus->parse() function to
get a binary device representation, and compare bytes per bytes the
binary representation given by your devargs and by the device->name.
But this is a hack, and a pretty ugly one at that: you have
no way of knowing the size taken by this binary representation, so you
can restrict yourself to the vdev and PCI bus for the moment and take
the larger of an rte_vdev_driver pointer and an rte_pci_addr....
{
union {
rte_vdev_driver *drv;
struct rte_pci_addr pci_addr;
} bindev1, bindev2;
memset(&bindev1, 0, sizeof(bindev1));
memset(&bindev2, 0, sizeof(bindev2));
rte_eal_devargs_parse(device->name, da1);
rte_eal_devargs_parse(your_devstr, da2);
RTE_ASSERT(da1->bus == rte_bus_find_by_name("pci") ||
da1->bus == rte_bus_find_by_name("vdev"));
RTE_ASSERT(da2->bus == rte_bus_find_by_name("pci") ||
da2->bus == rte_bus_find_by_name("vdev"));
da1->bus->parse(da1->name, &bindev1);
da1->bus->parse(da2->name, &bindev2);
if (memcmp(&bindev1, &bindev2, sizeof(bindev1)) == 0) {
/* found the device */
} else {
/* not found */
}
}
So, really, really ugly. Anyway.
<snip>
> > > + /* Take control of device probed by EAL options. */
> > > + DEBUG("Taking control of a probed sub device"
> > > + " %d named %s", i, da->name);
> >
> > In this case, the devargs of the probed device must be copied within the sub-
> > device definition and removed from the EAL using the proper rte_devargs
> > API.
> >
> > Note that there is no rte_devargs copy function. You can use
> > rte_devargs_parse instead, "parsing" again the original devargs into the sub-
> > device one. It is necessary for complying with internal rte_devargs
> > requirements (da->args being malloc-ed, at the moment, but may evolve).
> >
> > The rte_eal_devargs_parse function is not easy enough to use right now,
> > you will have to build a devargs string (using snprintf) and submit it.
> > I proposed a change this release for it but it will not make it for 18.02, that
> > would have simplified your implementation.
> >
>
> Got you. You right we need to remove the created devargs in fail-safe parse level.
> What do you think about checking it in the parse level and avoid the new devargs creation?
> Also to do the copy in parse level(same method as we are doing in probe level)?
>
Not sure I follow here, but the new rte_devargs is part of the
sub-device (it is not a pointer, but allocated alongside the
sub_device).
So keep everything here, it is the right place to deal with these
things.
Hi Gaetan
From: Gaëtan Rivet, Tuesday, January 16, 2018 4:41 PM
> On Tue, Jan 16, 2018 at 12:27:57PM +0000, Matan Azrad wrote:
> > Hi Gaetan
> >
> > From: Gaëtan Rivet, Tuesday, January 16, 2018 1:09 PM
> > > Hi Matan,
> > >
> > > I'n not fond of the commit title, how about:
> > >
> > > [PATCH v3 3/8] net/failsafe: add probed etherdev capture
> > >
> > > ?
> > >
> > OK, no problem.
> >
> > > On Tue, Jan 09, 2018 at 02:47:28PM +0000, Matan Azrad wrote:
> > > > Previous fail-safe code didn't support getting probed sub-devices
> > > > and failed when it tried to probe them.
> > > >
> > > > Skip fail-safe sub-device probing when it already was probed.
> > > >
> > > > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > > > Cc: Gaetan Rivet <gaetan.rivet@6wind.com>
> > > > ---
> > > > doc/guides/nics/fail_safe.rst | 5 ++++
> > > > drivers/net/failsafe/failsafe_eal.c | 60
> > > > ++++++++++++++++++++++++-------------
> > > > 2 files changed, 45 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/doc/guides/nics/fail_safe.rst
> > > > b/doc/guides/nics/fail_safe.rst index 5b1b47e..b89e53b 100644
> > > > --- a/doc/guides/nics/fail_safe.rst
> > > > +++ b/doc/guides/nics/fail_safe.rst
> > > > @@ -115,6 +115,11 @@ Fail-safe command line parameters
> > > > order to take only the last line into account (unlike ``exec()``) at every
> > > > probe attempt.
> > > >
> > > > +.. note::
> > > > +
> > > > + In case of whitelist sub-device probed by EAL, fail-safe PMD
> > > > + will take the
> > > device
> > > > + as is, which means that EAL device options are taken in this case.
> > > > +
> > > > - **mac** parameter [MAC address]
> > > >
> > > > This parameter allows the user to set a default MAC address to
> > > > the fail-safe diff --git a/drivers/net/failsafe/failsafe_eal.c
> > > > b/drivers/net/failsafe/failsafe_eal.c
> > > > index 19d26f5..7bc7453 100644
> > > > --- a/drivers/net/failsafe/failsafe_eal.c
> > > > +++ b/drivers/net/failsafe/failsafe_eal.c
> > > > @@ -36,39 +36,59 @@
> > > > #include "failsafe_private.h"
> > > >
> > > > static int
> > > > +fs_get_port_by_device_name(const char *name, uint16_t *port_id)
> > >
> > > The naming convention for the failsafe driver is
> > >
> > > namespace_object_sub-object_action()
> > >
> > OK.
> > > With an ordering of objects by their scope (std, rte, failsafe, file).
> > > Also, "get" as an action is not descriptive enough.
> > >
> > Isn't "get by device name" descriptive?
>
> The endgame is capturing a device that we know we are interested in.
> The device name being used for matching is an implementation detail, which
> should be abstracted by using a sub-function.
>
> Putting this in the name defeat the reason for using another function.
>
> > > static int
> > > fs_ethdev_capture(const char *name, uint16_t *port_id);
> > >
> > You miss here the main reason why we need this function instead of using
> rte_eth_dev_get_port_by_name.
> > The reason we need this function is because we want to find the device by
> the device name and not ethdev name.
> > What's about fs_port_capture_by_device_name?
>
> You are getting a port_id that is only valid for the rte_eth_devices array, by
> using the ethdev iterator. You are only looking for an ethdev.
>
> So it doesn't really matter whether you are using the ethdev name or the
> device name, in the end you are capturing an ethdev
> --> fs_ethdev_capture seems good for me.
>
I don't think so, this function doesn't take(capture) the device, just gets its ethdev port id using the device name.
The function which actually captures the device is the fs_bus_init.
So maybe even the "capture" name looks problematic here.
The main idea of this function is just to get the port_id.
> Now, I guess you will say that the user would need to know that they have to
> provide a device name that would be written in device->name. The issue
> here is that you have a leaky abstraction for your function, forcing this kind of
> consideration on your function user.
>
> So I'd go further and will ask you to change the `const char *name` to a `const
> rte_devargs *da` in the parameters.
>
> > Maybe comparing it to device->devargs->name is better, What do you
> think?
> >
>
> You are touching at a pretty contentious subject here :) .
>
> Identifying devices is not currently a well-defined function in DPDK.
> Some ports (actually, only one model: ConnectX-3) will have several ports
> using the same PCI slot. But even ignoring this glaring problem...
>
> As it is, the device->name for PCI will match the name given as a devargs, so
> functionally this should not change anything.
>
> Furthermore, you will have devices probed without any devargs. The fail-
> safe would thus be unable to capture non-blacklisted devices when the PCI
> bus is in blacklist mode.
>
> These not-blacklisted devices actually will have a full-PCI name (DomBDF
> format), so a simple match with the one passed in your fail-safe devargs will
> fail, ex:
>
> # A physical port exists at 0000:00:02.0
> testpmd --vdev="net_failsafe,dev(00:02.0)" -- -i
>
> Would fail to capture the device 0000:00:02.0, as this is the name that the PCI
> bus would give to this device, in the absence of a user-given name.
>
> In 18.05, or 18.08 there should be an EAL function that would be able to
> identify a device given a specific ID string (very close to an rte_devargs).
> Currently, this API does not exist.
>
> You can hack your way around this for the moment, IF you really, really
> want: parse your devargs, get the bus, use the bus->parse() function to get a
> binary device representation, and compare bytes per bytes the binary
> representation given by your devargs and by the device->name.
>
> But this is a hack, and a pretty ugly one at that: you have no way of knowing
> the size taken by this binary representation, so you can restrict yourself to
> the vdev and PCI bus for the moment and take the larger of an
> rte_vdev_driver pointer and an rte_pci_addr....
>
> {
> union {
> rte_vdev_driver *drv;
> struct rte_pci_addr pci_addr;
> } bindev1, bindev2;
> memset(&bindev1, 0, sizeof(bindev1));
> memset(&bindev2, 0, sizeof(bindev2));
> rte_eal_devargs_parse(device->name, da1);
> rte_eal_devargs_parse(your_devstr, da2);
> RTE_ASSERT(da1->bus == rte_bus_find_by_name("pci") ||
> da1->bus == rte_bus_find_by_name("vdev"));
> RTE_ASSERT(da2->bus == rte_bus_find_by_name("pci") ||
> da2->bus == rte_bus_find_by_name("vdev"));
> da1->bus->parse(da1->name, &bindev1);
> da1->bus->parse(da2->name, &bindev2);
> if (memcmp(&bindev1, &bindev2, sizeof(bindev1)) == 0) {
> /* found the device */
> } else {
> /* not found */
> }
> }
>
> So, really, really ugly. Anyway.
>
Yes, ugly :) Thanks for this update!
Will keep the comparison by device->name.
> <snip>
>
> > > > + /* Take control of device probed by EAL options. */
> > > > + DEBUG("Taking control of a probed sub device"
> > > > + " %d named %s", i, da->name);
> > >
> > > In this case, the devargs of the probed device must be copied within
> > > the sub- device definition and removed from the EAL using the proper
> > > rte_devargs API.
> > >
> > > Note that there is no rte_devargs copy function. You can use
> > > rte_devargs_parse instead, "parsing" again the original devargs into
> > > the sub- device one. It is necessary for complying with internal
> > > rte_devargs requirements (da->args being malloc-ed, at the moment,
> but may evolve).
> > >
> > > The rte_eal_devargs_parse function is not easy enough to use right
> > > now, you will have to build a devargs string (using snprintf) and submit it.
> > > I proposed a change this release for it but it will not make it for
> > > 18.02, that would have simplified your implementation.
> > >
> >
> > Got you. You right we need to remove the created devargs in fail-safe
> parse level.
> > What do you think about checking it in the parse level and avoid the new
> devargs creation?
> > Also to do the copy in parse level(same method as we are doing in probe
> level)?
> >
>
> Not sure I follow here, but the new rte_devargs is part of the sub-device (it is
> not a pointer, but allocated alongside the sub_device).
>
> So keep everything here, it is the right place to deal with these things.
>
But it will prevent the double parsing and also saves the method:
If the device already parsed - copy its devargs and continue.
If the device already probed - copy the device pointer and continue.
I think this is the right dealing, no?
Why to deal with parse level in probe level? Just keep all the parse work to parse level and the probe work to probe level.
Thanks, Matan.
> --
> Gaëtan Rivet
> 6WIND
On Tue, Jan 16, 2018 at 04:15:36PM +0000, Matan Azrad wrote:
> Hi Gaetan
>
> From: Gaëtan Rivet, Tuesday, January 16, 2018 4:41 PM
> > On Tue, Jan 16, 2018 at 12:27:57PM +0000, Matan Azrad wrote:
> > > Hi Gaetan
> > >
> > > From: Gaëtan Rivet, Tuesday, January 16, 2018 1:09 PM
> > > > Hi Matan,
> > > >
> > > > I'n not fond of the commit title, how about:
> > > >
> > > > [PATCH v3 3/8] net/failsafe: add probed etherdev capture
> > > >
> > > > ?
> > > >
> > > OK, no problem.
> > >
> > > > On Tue, Jan 09, 2018 at 02:47:28PM +0000, Matan Azrad wrote:
> > > > > Previous fail-safe code didn't support getting probed sub-devices
> > > > > and failed when it tried to probe them.
> > > > >
> > > > > Skip fail-safe sub-device probing when it already was probed.
> > > > >
> > > > > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > > > > Cc: Gaetan Rivet <gaetan.rivet@6wind.com>
> > > > > ---
> > > > > doc/guides/nics/fail_safe.rst | 5 ++++
> > > > > drivers/net/failsafe/failsafe_eal.c | 60
> > > > > ++++++++++++++++++++++++-------------
> > > > > 2 files changed, 45 insertions(+), 20 deletions(-)
> > > > >
> > > > > diff --git a/doc/guides/nics/fail_safe.rst
> > > > > b/doc/guides/nics/fail_safe.rst index 5b1b47e..b89e53b 100644
> > > > > --- a/doc/guides/nics/fail_safe.rst
> > > > > +++ b/doc/guides/nics/fail_safe.rst
> > > > > @@ -115,6 +115,11 @@ Fail-safe command line parameters
> > > > > order to take only the last line into account (unlike ``exec()``) at every
> > > > > probe attempt.
> > > > >
> > > > > +.. note::
> > > > > +
> > > > > + In case of whitelist sub-device probed by EAL, fail-safe PMD
> > > > > + will take the
> > > > device
> > > > > + as is, which means that EAL device options are taken in this case.
> > > > > +
> > > > > - **mac** parameter [MAC address]
> > > > >
> > > > > This parameter allows the user to set a default MAC address to
> > > > > the fail-safe diff --git a/drivers/net/failsafe/failsafe_eal.c
> > > > > b/drivers/net/failsafe/failsafe_eal.c
> > > > > index 19d26f5..7bc7453 100644
> > > > > --- a/drivers/net/failsafe/failsafe_eal.c
> > > > > +++ b/drivers/net/failsafe/failsafe_eal.c
> > > > > @@ -36,39 +36,59 @@
> > > > > #include "failsafe_private.h"
> > > > >
> > > > > static int
> > > > > +fs_get_port_by_device_name(const char *name, uint16_t *port_id)
> > > >
> > > > The naming convention for the failsafe driver is
> > > >
> > > > namespace_object_sub-object_action()
> > > >
> > > OK.
> > > > With an ordering of objects by their scope (std, rte, failsafe, file).
> > > > Also, "get" as an action is not descriptive enough.
> > > >
> > > Isn't "get by device name" descriptive?
> >
> > The endgame is capturing a device that we know we are interested in.
> > The device name being used for matching is an implementation detail, which
> > should be abstracted by using a sub-function.
> >
> > Putting this in the name defeat the reason for using another function.
> >
> > > > static int
> > > > fs_ethdev_capture(const char *name, uint16_t *port_id);
> > > >
> > > You miss here the main reason why we need this function instead of using
> > rte_eth_dev_get_port_by_name.
> > > The reason we need this function is because we want to find the device by
> > the device name and not ethdev name.
> > > What's about fs_port_capture_by_device_name?
> >
> > You are getting a port_id that is only valid for the rte_eth_devices array, by
> > using the ethdev iterator. You are only looking for an ethdev.
> >
> > So it doesn't really matter whether you are using the ethdev name or the
> > device name, in the end you are capturing an ethdev
> > --> fs_ethdev_capture seems good for me.
> >
>
> I don't think so, this function doesn't take(capture) the device, just gets its ethdev port id using the device name.
> The function which actually captures the device is the fs_bus_init.
> So maybe even the "capture" name looks problematic here.
> The main idea of this function is just to get the port_id.
>
Right :) . Call it fs_ethdev_portid_get() or fs_ethdev_find() then.
> > Now, I guess you will say that the user would need to know that they have to
> > provide a device name that would be written in device->name. The issue
> > here is that you have a leaky abstraction for your function, forcing this kind of
> > consideration on your function user.
> >
> > So I'd go further and will ask you to change the `const char *name` to a `const
> > rte_devargs *da` in the parameters.
> >
> > > Maybe comparing it to device->devargs->name is better, What do you
> > think?
> > >
> >
> > You are touching at a pretty contentious subject here :) .
> >
> > Identifying devices is not currently a well-defined function in DPDK.
> > Some ports (actually, only one model: ConnectX-3) will have several ports
> > using the same PCI slot. But even ignoring this glaring problem...
> >
> > As it is, the device->name for PCI will match the name given as a devargs, so
> > functionally this should not change anything.
> >
> > Furthermore, you will have devices probed without any devargs. The fail-
> > safe would thus be unable to capture non-blacklisted devices when the PCI
> > bus is in blacklist mode.
> >
> > These not-blacklisted devices actually will have a full-PCI name (DomBDF
> > format), so a simple match with the one passed in your fail-safe devargs will
> > fail, ex:
> >
> > # A physical port exists at 0000:00:02.0
> > testpmd --vdev="net_failsafe,dev(00:02.0)" -- -i
> >
> > Would fail to capture the device 0000:00:02.0, as this is the name that the PCI
> > bus would give to this device, in the absence of a user-given name.
> >
> > In 18.05, or 18.08 there should be an EAL function that would be able to
> > identify a device given a specific ID string (very close to an rte_devargs).
> > Currently, this API does not exist.
> >
> > You can hack your way around this for the moment, IF you really, really
> > want: parse your devargs, get the bus, use the bus->parse() function to get a
> > binary device representation, and compare bytes per bytes the binary
> > representation given by your devargs and by the device->name.
> >
> > But this is a hack, and a pretty ugly one at that: you have no way of knowing
> > the size taken by this binary representation, so you can restrict yourself to
> > the vdev and PCI bus for the moment and take the larger of an
> > rte_vdev_driver pointer and an rte_pci_addr....
> >
> > {
> > union {
> > rte_vdev_driver *drv;
> > struct rte_pci_addr pci_addr;
> > } bindev1, bindev2;
> > memset(&bindev1, 0, sizeof(bindev1));
> > memset(&bindev2, 0, sizeof(bindev2));
> > rte_eal_devargs_parse(device->name, da1);
> > rte_eal_devargs_parse(your_devstr, da2);
> > RTE_ASSERT(da1->bus == rte_bus_find_by_name("pci") ||
> > da1->bus == rte_bus_find_by_name("vdev"));
> > RTE_ASSERT(da2->bus == rte_bus_find_by_name("pci") ||
> > da2->bus == rte_bus_find_by_name("vdev"));
> > da1->bus->parse(da1->name, &bindev1);
> > da1->bus->parse(da2->name, &bindev2);
> > if (memcmp(&bindev1, &bindev2, sizeof(bindev1)) == 0) {
> > /* found the device */
> > } else {
> > /* not found */
> > }
> > }
> >
> > So, really, really ugly. Anyway.
> >
> Yes, ugly :) Thanks for this update!
> Will keep the comparison by device->name.
>
Well as explained, above, the comparison by device->name only works with
whitelisted devices.
So either implement something broken right now that you will need to
update in 18.05, or implement it properly in 18.05 from the get go.
> > <snip>
> >
> > > > > + /* Take control of device probed by EAL options. */
> > > > > + DEBUG("Taking control of a probed sub device"
> > > > > + " %d named %s", i, da->name);
> > > >
> > > > In this case, the devargs of the probed device must be copied within
> > > > the sub- device definition and removed from the EAL using the proper
> > > > rte_devargs API.
> > > >
> > > > Note that there is no rte_devargs copy function. You can use
> > > > rte_devargs_parse instead, "parsing" again the original devargs into
> > > > the sub- device one. It is necessary for complying with internal
> > > > rte_devargs requirements (da->args being malloc-ed, at the moment,
> > but may evolve).
> > > >
> > > > The rte_eal_devargs_parse function is not easy enough to use right
> > > > now, you will have to build a devargs string (using snprintf) and submit it.
> > > > I proposed a change this release for it but it will not make it for
> > > > 18.02, that would have simplified your implementation.
> > > >
> > >
> > > Got you. You right we need to remove the created devargs in fail-safe
> > parse level.
> > > What do you think about checking it in the parse level and avoid the new
> > devargs creation?
> > > Also to do the copy in parse level(same method as we are doing in probe
> > level)?
> > >
> >
> > Not sure I follow here, but the new rte_devargs is part of the sub-device (it is
> > not a pointer, but allocated alongside the sub_device).
> >
> > So keep everything here, it is the right place to deal with these things.
> >
> But it will prevent the double parsing and also saves the method:
> If the device already parsed - copy its devargs and continue.
> If the device already probed - copy the device pointer and continue.
>
> I think this is the right dealing, no?
> Why to deal with parse level in probe level? Just keep all the parse work to parse level and the probe work to probe level.
After re-reading, I think we misunderstood each other.
You cannot remove the rte_devargs created during parsing: it is
allocated alongside the sub_device structure.
You must only remove the rte_devargs allocated by the EAL (using
rte_eal_devargs_remove()).
Before removing it, you must copy its content in the local sub_device
rte_devargs structure. I only proposed a way to do this copy that would
not deal with rte_devargs internals, as it is bound to evolve rather
soon.
Otherwise, no, I do not want to complicate the parsing operations, they
are already too complicated and too criticals. Better to keep it all
here.
Hi Gaetan
From: Gaëtan Rivet, Tuesday, January 16, 2018 6:54 PM
> On Tue, Jan 16, 2018 at 04:15:36PM +0000, Matan Azrad wrote:
> > Hi Gaetan
> >
> > From: Gaëtan Rivet, Tuesday, January 16, 2018 4:41 PM
> > > On Tue, Jan 16, 2018 at 12:27:57PM +0000, Matan Azrad wrote:
> > > > Hi Gaetan
> > > >
> > > > From: Gaëtan Rivet, Tuesday, January 16, 2018 1:09 PM
> > > > > Hi Matan,
> > > > >
> > > > > I'n not fond of the commit title, how about:
> > > > >
> > > > > [PATCH v3 3/8] net/failsafe: add probed etherdev capture
> > > > >
> > > > > ?
> > > > >
> > > > OK, no problem.
> > > >
> > > > > On Tue, Jan 09, 2018 at 02:47:28PM +0000, Matan Azrad wrote:
> > > > > > Previous fail-safe code didn't support getting probed
> > > > > > sub-devices and failed when it tried to probe them.
> > > > > >
> > > > > > Skip fail-safe sub-device probing when it already was probed.
> > > > > >
> > > > > > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > > > > > Cc: Gaetan Rivet <gaetan.rivet@6wind.com>
> > > > > > ---
> > > > > > doc/guides/nics/fail_safe.rst | 5 ++++
> > > > > > drivers/net/failsafe/failsafe_eal.c | 60
> > > > > > ++++++++++++++++++++++++-------------
> > > > > > 2 files changed, 45 insertions(+), 20 deletions(-)
> > > > > >
> > > > > > diff --git a/doc/guides/nics/fail_safe.rst
> > > > > > b/doc/guides/nics/fail_safe.rst index 5b1b47e..b89e53b 100644
> > > > > > --- a/doc/guides/nics/fail_safe.rst
> > > > > > +++ b/doc/guides/nics/fail_safe.rst
> > > > > > @@ -115,6 +115,11 @@ Fail-safe command line parameters
> > > > > > order to take only the last line into account (unlike ``exec()``) at
> every
> > > > > > probe attempt.
> > > > > >
> > > > > > +.. note::
> > > > > > +
> > > > > > + In case of whitelist sub-device probed by EAL, fail-safe
> > > > > > + PMD will take the
> > > > > device
> > > > > > + as is, which means that EAL device options are taken in this case.
> > > > > > +
> > > > > > - **mac** parameter [MAC address]
> > > > > >
> > > > > > This parameter allows the user to set a default MAC address
> > > > > > to the fail-safe diff --git
> > > > > > a/drivers/net/failsafe/failsafe_eal.c
> > > > > > b/drivers/net/failsafe/failsafe_eal.c
> > > > > > index 19d26f5..7bc7453 100644
> > > > > > --- a/drivers/net/failsafe/failsafe_eal.c
> > > > > > +++ b/drivers/net/failsafe/failsafe_eal.c
> > > > > > @@ -36,39 +36,59 @@
> > > > > > #include "failsafe_private.h"
> > > > > >
> > > > > > static int
> > > > > > +fs_get_port_by_device_name(const char *name, uint16_t
> > > > > > +*port_id)
> > > > >
> > > > > The naming convention for the failsafe driver is
> > > > >
> > > > > namespace_object_sub-object_action()
> > > > >
> > > > OK.
> > > > > With an ordering of objects by their scope (std, rte, failsafe, file).
> > > > > Also, "get" as an action is not descriptive enough.
> > > > >
> > > > Isn't "get by device name" descriptive?
> > >
> > > The endgame is capturing a device that we know we are interested in.
> > > The device name being used for matching is an implementation detail,
> > > which should be abstracted by using a sub-function.
> > >
> > > Putting this in the name defeat the reason for using another function.
> > >
> > > > > static int
> > > > > fs_ethdev_capture(const char *name, uint16_t *port_id);
> > > > >
> > > > You miss here the main reason why we need this function instead of
> > > > using
> > > rte_eth_dev_get_port_by_name.
> > > > The reason we need this function is because we want to find the
> > > > device by
> > > the device name and not ethdev name.
> > > > What's about fs_port_capture_by_device_name?
> > >
> > > You are getting a port_id that is only valid for the rte_eth_devices
> > > array, by using the ethdev iterator. You are only looking for an ethdev.
> > >
> > > So it doesn't really matter whether you are using the ethdev name or
> > > the device name, in the end you are capturing an ethdev
> > > --> fs_ethdev_capture seems good for me.
> > >
> >
> > I don't think so, this function doesn't take(capture) the device, just gets its
> ethdev port id using the device name.
> > The function which actually captures the device is the fs_bus_init.
> > So maybe even the "capture" name looks problematic here.
> > The main idea of this function is just to get the port_id.
> >
>
> Right :) . Call it fs_ethdev_portid_get() or fs_ethdev_find() then.
>
Sure, agree with the first one.
> > > Now, I guess you will say that the user would need to know that they
> > > have to provide a device name that would be written in device->name.
> > > The issue here is that you have a leaky abstraction for your
> > > function, forcing this kind of consideration on your function user.
> > >
> > > So I'd go further and will ask you to change the `const char *name`
> > > to a `const rte_devargs *da` in the parameters.
> > >
> > > > Maybe comparing it to device->devargs->name is better, What do you
> > > think?
> > > >
> > >
> > > You are touching at a pretty contentious subject here :) .
> > >
> > > Identifying devices is not currently a well-defined function in DPDK.
> > > Some ports (actually, only one model: ConnectX-3) will have several
> > > ports using the same PCI slot. But even ignoring this glaring problem...
> > >
> > > As it is, the device->name for PCI will match the name given as a
> > > devargs, so functionally this should not change anything.
> > >
> > > Furthermore, you will have devices probed without any devargs. The
> > > fail- safe would thus be unable to capture non-blacklisted devices
> > > when the PCI bus is in blacklist mode.
> > >
> > > These not-blacklisted devices actually will have a full-PCI name
> > > (DomBDF format), so a simple match with the one passed in your
> > > fail-safe devargs will fail, ex:
> > >
> > > # A physical port exists at 0000:00:02.0
> > > testpmd --vdev="net_failsafe,dev(00:02.0)" -- -i
> > >
> > > Would fail to capture the device 0000:00:02.0, as this is the name
> > > that the PCI bus would give to this device, in the absence of a user-given
> name.
> > >
> > > In 18.05, or 18.08 there should be an EAL function that would be
> > > able to identify a device given a specific ID string (very close to an
> rte_devargs).
> > > Currently, this API does not exist.
> > >
> > > You can hack your way around this for the moment, IF you really,
> > > really
> > > want: parse your devargs, get the bus, use the bus->parse() function
> > > to get a binary device representation, and compare bytes per bytes
> > > the binary representation given by your devargs and by the device-
> >name.
> > >
> > > But this is a hack, and a pretty ugly one at that: you have no way
> > > of knowing the size taken by this binary representation, so you can
> > > restrict yourself to the vdev and PCI bus for the moment and take
> > > the larger of an rte_vdev_driver pointer and an rte_pci_addr....
> > >
> > > {
> > > union {
> > > rte_vdev_driver *drv;
> > > struct rte_pci_addr pci_addr;
> > > } bindev1, bindev2;
> > > memset(&bindev1, 0, sizeof(bindev1));
> > > memset(&bindev2, 0, sizeof(bindev2));
> > > rte_eal_devargs_parse(device->name, da1);
> > > rte_eal_devargs_parse(your_devstr, da2);
> > > RTE_ASSERT(da1->bus == rte_bus_find_by_name("pci") ||
> > > da1->bus == rte_bus_find_by_name("vdev"));
> > > RTE_ASSERT(da2->bus == rte_bus_find_by_name("pci") ||
> > > da2->bus == rte_bus_find_by_name("vdev"));
> > > da1->bus->parse(da1->name, &bindev1);
> > > da1->bus->parse(da2->name, &bindev2);
> > > if (memcmp(&bindev1, &bindev2, sizeof(bindev1)) == 0) {
> > > /* found the device */
> > > } else {
> > > /* not found */
> > > }
> > > }
> > >
> > > So, really, really ugly. Anyway.
> > >
> > Yes, ugly :) Thanks for this update!
> > Will keep the comparison by device->name.
> >
>
> Well as explained, above, the comparison by device->name only works with
> whitelisted devices.
>
> So either implement something broken right now that you will need to
> update in 18.05, or implement it properly in 18.05 from the get go.
>
For the current needs it is enough.
We can also say that it is the user responsibility to pass to failsafe the same names and same args as he passes for EAL(or default EAL names).
I think I emphasized it in documentation.
> > > <snip>
> > >
> > > > > > + /* Take control of device probed by EAL
> options. */
> > > > > > + DEBUG("Taking control of a probed sub
> device"
> > > > > > + " %d named %s", i, da->name);
> > > > >
> > > > > In this case, the devargs of the probed device must be copied
> > > > > within the sub- device definition and removed from the EAL using
> > > > > the proper rte_devargs API.
> > > > >
> > > > > Note that there is no rte_devargs copy function. You can use
> > > > > rte_devargs_parse instead, "parsing" again the original devargs
> > > > > into the sub- device one. It is necessary for complying with
> > > > > internal rte_devargs requirements (da->args being malloc-ed, at
> > > > > the moment,
> > > but may evolve).
> > > > >
> > > > > The rte_eal_devargs_parse function is not easy enough to use
> > > > > right now, you will have to build a devargs string (using snprintf) and
> submit it.
> > > > > I proposed a change this release for it but it will not make it
> > > > > for 18.02, that would have simplified your implementation.
> > > > >
> > > >
> > > > Got you. You right we need to remove the created devargs in
> > > > fail-safe
> > > parse level.
> > > > What do you think about checking it in the parse level and avoid
> > > > the new
> > > devargs creation?
> > > > Also to do the copy in parse level(same method as we are doing in
> > > > probe
> > > level)?
> > > >
> > >
> > > Not sure I follow here, but the new rte_devargs is part of the
> > > sub-device (it is not a pointer, but allocated alongside the sub_device).
> > >
> > > So keep everything here, it is the right place to deal with these things.
> > >
> > But it will prevent the double parsing and also saves the method:
> > If the device already parsed - copy its devargs and continue.
> > If the device already probed - copy the device pointer and continue.
> >
> > I think this is the right dealing, no?
> > Why to deal with parse level in probe level? Just keep all the parse work to
> parse level and the probe work to probe level.
>
> After re-reading, I think we misunderstood each other.
> You cannot remove the rte_devargs created during parsing: it is allocated
> alongside the sub_device structure.
>
> You must only remove the rte_devargs allocated by the EAL (using
> rte_eal_devargs_remove()).
>
Sure.
> Before removing it, you must copy its content in the local sub_device
> rte_devargs structure. I only proposed a way to do this copy that would not
> deal with rte_devargs internals, as it is bound to evolve rather soon.
>
Yes.
> Otherwise, no, I do not want to complicate the parsing operations, they are
> already too complicated and too criticals. Better to keep it all here.
I think fs_parse_device function is not complicated and it is the natural place for devargs games.
For me this is the right place for the copy & remove devargs.
Are you insisting to put all in fs_bus_init?
> --
> Gaëtan Rivet
> 6WIND
Hi Matan,
On Tue, Jan 16, 2018 at 05:20:27PM +0000, Matan Azrad wrote:
> Hi Gaetan
>
<snip>
> > > > In 18.05, or 18.08 there should be an EAL function that would be
> > > > able to identify a device given a specific ID string (very close to an
> > rte_devargs).
> > > > Currently, this API does not exist.
> > > >
> > > > You can hack your way around this for the moment, IF you really,
> > > > really
> > > > want: parse your devargs, get the bus, use the bus->parse() function
> > > > to get a binary device representation, and compare bytes per bytes
> > > > the binary representation given by your devargs and by the device-
> > >name.
> > > >
> > > > But this is a hack, and a pretty ugly one at that: you have no way
> > > > of knowing the size taken by this binary representation, so you can
> > > > restrict yourself to the vdev and PCI bus for the moment and take
> > > > the larger of an rte_vdev_driver pointer and an rte_pci_addr....
> > > >
> > > > {
> > > > union {
> > > > rte_vdev_driver *drv;
> > > > struct rte_pci_addr pci_addr;
> > > > } bindev1, bindev2;
> > > > memset(&bindev1, 0, sizeof(bindev1));
> > > > memset(&bindev2, 0, sizeof(bindev2));
> > > > rte_eal_devargs_parse(device->name, da1);
> > > > rte_eal_devargs_parse(your_devstr, da2);
> > > > RTE_ASSERT(da1->bus == rte_bus_find_by_name("pci") ||
> > > > da1->bus == rte_bus_find_by_name("vdev"));
> > > > RTE_ASSERT(da2->bus == rte_bus_find_by_name("pci") ||
> > > > da2->bus == rte_bus_find_by_name("vdev"));
> > > > da1->bus->parse(da1->name, &bindev1);
> > > > da1->bus->parse(da2->name, &bindev2);
> > > > if (memcmp(&bindev1, &bindev2, sizeof(bindev1)) == 0) {
> > > > /* found the device */
> > > > } else {
> > > > /* not found */
> > > > }
> > > > }
> > > >
> > > > So, really, really ugly. Anyway.
> > > >
> > > Yes, ugly :) Thanks for this update!
> > > Will keep the comparison by device->name.
> > >
> >
> > Well as explained, above, the comparison by device->name only works with
> > whitelisted devices.
> >
>
> > So either implement something broken right now that you will need to
> > update in 18.05, or implement it properly in 18.05 from the get go.
> >
> For the current needs it is enough.
> We can also say that it is the user responsibility to pass to failsafe the same names and same args as he passes for EAL(or default EAL names).
> I think I emphasized it in documentation.
>
Okay, as you wish. Just be aware of this limitation.
I think this functionality is good and useful, but it needs to be made clean.
The proper function should be available soon, then this implementaion should
be cleaned up.
> > > > <snip>
> > > >
> > > > > > > + /* Take control of device probed by EAL
> > options. */
> > > > > > > + DEBUG("Taking control of a probed sub
> > device"
> > > > > > > + " %d named %s", i, da->name);
> > > > > >
> > > > > > In this case, the devargs of the probed device must be copied
> > > > > > within the sub- device definition and removed from the EAL using
> > > > > > the proper rte_devargs API.
> > > > > >
> > > > > > Note that there is no rte_devargs copy function. You can use
> > > > > > rte_devargs_parse instead, "parsing" again the original devargs
> > > > > > into the sub- device one. It is necessary for complying with
> > > > > > internal rte_devargs requirements (da->args being malloc-ed, at
> > > > > > the moment,
> > > > but may evolve).
> > > > > >
> > > > > > The rte_eal_devargs_parse function is not easy enough to use
> > > > > > right now, you will have to build a devargs string (using snprintf) and
> > submit it.
> > > > > > I proposed a change this release for it but it will not make it
> > > > > > for 18.02, that would have simplified your implementation.
> > > > > >
> > > > >
> > > > > Got you. You right we need to remove the created devargs in
> > > > > fail-safe
> > > > parse level.
> > > > > What do you think about checking it in the parse level and avoid
> > > > > the new
> > > > devargs creation?
> > > > > Also to do the copy in parse level(same method as we are doing in
> > > > > probe
> > > > level)?
> > > > >
> > > >
> > > > Not sure I follow here, but the new rte_devargs is part of the
> > > > sub-device (it is not a pointer, but allocated alongside the sub_device).
> > > >
> > > > So keep everything here, it is the right place to deal with these things.
> > > >
> > > But it will prevent the double parsing and also saves the method:
> > > If the device already parsed - copy its devargs and continue.
> > > If the device already probed - copy the device pointer and continue.
> > >
> > > I think this is the right dealing, no?
> > > Why to deal with parse level in probe level? Just keep all the parse work to
> > parse level and the probe work to probe level.
> >
> > After re-reading, I think we misunderstood each other.
> > You cannot remove the rte_devargs created during parsing: it is allocated
> > alongside the sub_device structure.
> >
> > You must only remove the rte_devargs allocated by the EAL (using
> > rte_eal_devargs_remove()).
> >
>
> Sure.
>
> > Before removing it, you must copy its content in the local sub_device
> > rte_devargs structure. I only proposed a way to do this copy that would not
> > deal with rte_devargs internals, as it is bound to evolve rather soon.
> >
> Yes.
>
> > Otherwise, no, I do not want to complicate the parsing operations, they are
> > already too complicated and too criticals. Better to keep it all here.
>
> I think fs_parse_device function is not complicated and it is the natural place for devargs games.
> For me this is the right place for the copy & remove devargs.
> Are you insisting to put all in fs_bus_init?
You would have to put fs_ethdev_portid_find in failsafe_args, which is
mixing layers. Sorry but yes, please keep all these changes in this
file.
Thanks,
Hi Gaetan
From: Gaëtan Rivet, Wednesday, January 17, 2018 12:31 AM
> Hi Matan,
>
> On Tue, Jan 16, 2018 at 05:20:27PM +0000, Matan Azrad wrote:
> > Hi Gaetan
> >
>
> <snip>
>
> > > > > In 18.05, or 18.08 there should be an EAL function that would be
> > > > > able to identify a device given a specific ID string (very close
> > > > > to an
> > > rte_devargs).
> > > > > Currently, this API does not exist.
> > > > >
> > > > > You can hack your way around this for the moment, IF you really,
> > > > > really
> > > > > want: parse your devargs, get the bus, use the bus->parse()
> > > > > function to get a binary device representation, and compare
> > > > > bytes per bytes the binary representation given by your devargs
> > > > > and by the device-
> > > >name.
> > > > >
> > > > > But this is a hack, and a pretty ugly one at that: you have no
> > > > > way of knowing the size taken by this binary representation, so
> > > > > you can restrict yourself to the vdev and PCI bus for the moment
> > > > > and take the larger of an rte_vdev_driver pointer and an
> rte_pci_addr....
> > > > >
> > > > > {
> > > > > union {
> > > > > rte_vdev_driver *drv;
> > > > > struct rte_pci_addr pci_addr;
> > > > > } bindev1, bindev2;
> > > > > memset(&bindev1, 0, sizeof(bindev1));
> > > > > memset(&bindev2, 0, sizeof(bindev2));
> > > > > rte_eal_devargs_parse(device->name, da1);
> > > > > rte_eal_devargs_parse(your_devstr, da2);
> > > > > RTE_ASSERT(da1->bus == rte_bus_find_by_name("pci") ||
> > > > > da1->bus == rte_bus_find_by_name("vdev"));
> > > > > RTE_ASSERT(da2->bus == rte_bus_find_by_name("pci") ||
> > > > > da2->bus == rte_bus_find_by_name("vdev"));
> > > > > da1->bus->parse(da1->name, &bindev1);
> > > > > da1->bus->parse(da2->name, &bindev2);
> > > > > if (memcmp(&bindev1, &bindev2, sizeof(bindev1)) == 0) {
> > > > > /* found the device */
> > > > > } else {
> > > > > /* not found */
> > > > > }
> > > > > }
> > > > >
> > > > > So, really, really ugly. Anyway.
> > > > >
> > > > Yes, ugly :) Thanks for this update!
> > > > Will keep the comparison by device->name.
> > > >
> > >
> > > Well as explained, above, the comparison by device->name only works
> > > with whitelisted devices.
> > >
> >
> > > So either implement something broken right now that you will need to
> > > update in 18.05, or implement it properly in 18.05 from the get go.
> > >
> > For the current needs it is enough.
> > We can also say that it is the user responsibility to pass to failsafe the same
> names and same args as he passes for EAL(or default EAL names).
> > I think I emphasized it in documentation.
> >
>
> Okay, as you wish. Just be aware of this limitation.
>
> I think this functionality is good and useful, but it needs to be made clean.
> The proper function should be available soon, then this implementaion
> should be cleaned up.
Sure.
>
> > > > > <snip>
> > > > >
> > > > > > > > + /* Take control of device probed by EAL
> > > options. */
> > > > > > > > + DEBUG("Taking control of a probed sub
> > > device"
> > > > > > > > + " %d named %s", i, da->name);
> > > > > > >
> > > > > > > In this case, the devargs of the probed device must be
> > > > > > > copied within the sub- device definition and removed from
> > > > > > > the EAL using the proper rte_devargs API.
> > > > > > >
> > > > > > > Note that there is no rte_devargs copy function. You can use
> > > > > > > rte_devargs_parse instead, "parsing" again the original
> > > > > > > devargs into the sub- device one. It is necessary for
> > > > > > > complying with internal rte_devargs requirements (da->args
> > > > > > > being malloc-ed, at the moment,
> > > > > but may evolve).
> > > > > > >
> > > > > > > The rte_eal_devargs_parse function is not easy enough to use
> > > > > > > right now, you will have to build a devargs string (using
> > > > > > > snprintf) and
> > > submit it.
> > > > > > > I proposed a change this release for it but it will not make
> > > > > > > it for 18.02, that would have simplified your implementation.
> > > > > > >
> > > > > >
> > > > > > Got you. You right we need to remove the created devargs in
> > > > > > fail-safe
> > > > > parse level.
> > > > > > What do you think about checking it in the parse level and
> > > > > > avoid the new
> > > > > devargs creation?
> > > > > > Also to do the copy in parse level(same method as we are doing
> > > > > > in probe
> > > > > level)?
> > > > > >
> > > > >
> > > > > Not sure I follow here, but the new rte_devargs is part of the
> > > > > sub-device (it is not a pointer, but allocated alongside the
> sub_device).
> > > > >
> > > > > So keep everything here, it is the right place to deal with these things.
> > > > >
> > > > But it will prevent the double parsing and also saves the method:
> > > > If the device already parsed - copy its devargs and continue.
> > > > If the device already probed - copy the device pointer and continue.
> > > >
> > > > I think this is the right dealing, no?
> > > > Why to deal with parse level in probe level? Just keep all the
> > > > parse work to
> > > parse level and the probe work to probe level.
> > >
> > > After re-reading, I think we misunderstood each other.
> > > You cannot remove the rte_devargs created during parsing: it is
> > > allocated alongside the sub_device structure.
> > >
> > > You must only remove the rte_devargs allocated by the EAL (using
> > > rte_eal_devargs_remove()).
> > >
> >
> > Sure.
> >
> > > Before removing it, you must copy its content in the local
> > > sub_device rte_devargs structure. I only proposed a way to do this
> > > copy that would not deal with rte_devargs internals, as it is bound to
> evolve rather soon.
> > >
> > Yes.
> >
> > > Otherwise, no, I do not want to complicate the parsing operations,
> > > they are already too complicated and too criticals. Better to keep it all
> here.
> >
> > I think fs_parse_device function is not complicated and it is the natural
> place for devargs games.
> > For me this is the right place for the copy & remove devargs.
> > Are you insisting to put all in fs_bus_init?
>
> You would have to put fs_ethdev_portid_find in failsafe_args, which is
> mixing layers. Sorry but yes, please keep all these changes in this file.
>
OK, Thanks man!
> Thanks,
> --
> Gaëtan Rivet
> 6WIND
@@ -115,6 +115,11 @@ Fail-safe command line parameters
order to take only the last line into account (unlike ``exec()``) at every
probe attempt.
+.. note::
+
+ In case of whitelist sub-device probed by EAL, fail-safe PMD will take the device
+ as is, which means that EAL device options are taken in this case.
+
- **mac** parameter [MAC address]
This parameter allows the user to set a default MAC address to the fail-safe
@@ -36,39 +36,59 @@
#include "failsafe_private.h"
static int
+fs_get_port_by_device_name(const char *name, uint16_t *port_id)
+{
+ uint16_t pid;
+ size_t len;
+
+ if (name == NULL) {
+ DEBUG("Null pointer is specified\n");
+ return -EINVAL;
+ }
+ len = strlen(name);
+ RTE_ETH_FOREACH_DEV(pid) {
+ if (!strncmp(name, rte_eth_devices[pid].device->name, len)) {
+ *port_id = pid;
+ return 0;
+ }
+ }
+ return -ENODEV;
+}
+
+static int
fs_bus_init(struct rte_eth_dev *dev)
{
struct sub_device *sdev;
struct rte_devargs *da;
uint8_t i;
- uint16_t j;
+ uint16_t pid;
int ret;
FOREACH_SUBDEV(sdev, i, dev) {
if (sdev->state != DEV_PARSED)
continue;
da = &sdev->devargs;
- ret = rte_eal_hotplug_add(da->bus->name,
- da->name,
- da->args);
- if (ret) {
- ERROR("sub_device %d probe failed %s%s%s", i,
- rte_errno ? "(" : "",
- rte_errno ? strerror(rte_errno) : "",
- rte_errno ? ")" : "");
- continue;
- }
- RTE_ETH_FOREACH_DEV(j) {
- if (strcmp(rte_eth_devices[j].device->name,
- da->name) == 0) {
- ETH(sdev) = &rte_eth_devices[j];
- break;
+ if (fs_get_port_by_device_name(da->name, &pid) != 0) {
+ ret = rte_eal_hotplug_add(da->bus->name,
+ da->name,
+ da->args);
+ if (ret) {
+ ERROR("sub_device %d probe failed %s%s%s", i,
+ rte_errno ? "(" : "",
+ rte_errno ? strerror(rte_errno) : "",
+ rte_errno ? ")" : "");
+ continue;
}
+ if (fs_get_port_by_device_name(da->name, &pid) != 0) {
+ ERROR("sub_device %d init went wrong", i);
+ return -ENODEV;
+ }
+ } else {
+ /* Take control of device probed by EAL options. */
+ DEBUG("Taking control of a probed sub device"
+ " %d named %s", i, da->name);
}
- if (ETH(sdev) == NULL) {
- ERROR("sub_device %d init went wrong", i);
- return -ENODEV;
- }
+ ETH(sdev) = &rte_eth_devices[pid];
SUB_ID(sdev) = i;
sdev->fs_dev = dev;
sdev->dev = ETH(sdev)->device;