diff mbox series

bus/vdev: automatically add eth alias for net drivers

Message ID 20221019131118.32394-1-bruce.richardson@intel.com (mailing list archive)
State Rejected
Delegated to: Thomas Monjalon
Headers show
Series bus/vdev: automatically add eth alias for net drivers | expand

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS

Commit Message

Bruce Richardson Oct. 19, 2022, 1:11 p.m. UTC
For historical reasons, a number of net vdev drivers also add a driver
alias using the "eth_" prefix. Since this is done on a per-driver basis,
the use of the alias in inconsistent and is spread across multiple
files. We can remove the per-driver aliases by just adding the alias
automatically at the vdev bus level.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/bus/vdev/vdev.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Bruce Richardson Oct. 19, 2022, 1:20 p.m. UTC | #1
On Wed, Oct 19, 2022 at 02:11:18PM +0100, Bruce Richardson wrote:
> For historical reasons, a number of net vdev drivers also add a driver
> alias using the "eth_" prefix. Since this is done on a per-driver basis,
> the use of the alias in inconsistent and is spread across multiple
> files. We can remove the per-driver aliases by just adding the alias
> automatically at the vdev bus level.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  drivers/bus/vdev/vdev.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
> index f5b43f1930..bfd7ce60c1 100644
> --- a/drivers/bus/vdev/vdev.c
> +++ b/drivers/bus/vdev/vdev.c
> @@ -54,6 +54,12 @@ static rte_spinlock_t vdev_custom_scan_lock = RTE_SPINLOCK_INITIALIZER;
>  void
>  rte_vdev_register(struct rte_vdev_driver *driver)
>  {
> +	/* For net driver vdevs, add an automatic alias using "eth" prefix */
> +	if (strncmp(driver->driver.name, "net_", 4) == 0 && driver->driver.alias == NULL) {
> +		char *alias = strdup(driver->driver.name);
> +		memcpy(alias, "eth_", 4);
> +		driver->driver.alias = alias;
> +	}
>  	TAILQ_INSERT_TAIL(&vdev_driver_list, driver, next);
>  }
>  

As a self-review comment, I realise this solution has got an issue that it
leaks memory if drivers are constantly being registered or unregistered. I
find it hard to see situations where this can occur, but it is a potential
issue.

A second solution that does not have this problem is to move the aliasing
to EAL, as below:

index fb5d0a293b..37b86914a0 100644
--- a/lib/eal/common/eal_common_devargs.c
+++ b/lib/eal/common/eal_common_devargs.c
@@ -226,9 +226,14 @@ rte_devargs_parse(struct rte_devargs *da, const char *dev)
        da->name[i] = '\0';
        if (bus == NULL) {
                bus = rte_bus_find_by_device_name(da->name);
+               if (bus == NULL && strncmp(da->name, "eth_", 4) == 0) {
+                       RTE_LOG(INFO, EAL, "failed to parse device \"%s\"...\n", da->name);
+                       memcpy(da->name, "net_", 4);
+                       RTE_LOG(INFO, EAL, "... trying device \"%s\"\n", da->name);
+                       bus = rte_bus_find_by_device_name(da->name);
+               }
                if (bus == NULL) {
                       RTE_LOG(ERR, EAL, "failed to parse device \"%s\"\n",
                               da->name);

While this doesn't have a memory freeing issue, it's downside is obviously
that we have further abstraction leakage, from the vdev bus for net drivers
all the way to EAL. Again, since the code delta is fairly small, this may
be acceptable, so opinions welcome.

/Bruce
Thomas Monjalon Oct. 20, 2022, 8:23 a.m. UTC | #2
19/10/2022 15:20, Bruce Richardson:
> On Wed, Oct 19, 2022 at 02:11:18PM +0100, Bruce Richardson wrote:
> > For historical reasons, a number of net vdev drivers also add a driver
> > alias using the "eth_" prefix. Since this is done on a per-driver basis,
> > the use of the alias in inconsistent and is spread across multiple
> > files. We can remove the per-driver aliases by just adding the alias
> > automatically at the vdev bus level.
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  drivers/bus/vdev/vdev.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
> > index f5b43f1930..bfd7ce60c1 100644
> > --- a/drivers/bus/vdev/vdev.c
> > +++ b/drivers/bus/vdev/vdev.c
> > @@ -54,6 +54,12 @@ static rte_spinlock_t vdev_custom_scan_lock = RTE_SPINLOCK_INITIALIZER;
> >  void
> >  rte_vdev_register(struct rte_vdev_driver *driver)
> >  {
> > +	/* For net driver vdevs, add an automatic alias using "eth" prefix */
> > +	if (strncmp(driver->driver.name, "net_", 4) == 0 && driver->driver.alias == NULL) {
> > +		char *alias = strdup(driver->driver.name);
> > +		memcpy(alias, "eth_", 4);
> > +		driver->driver.alias = alias;
> > +	}
> >  	TAILQ_INSERT_TAIL(&vdev_driver_list, driver, next);
> >  }
> >  
> 
> As a self-review comment, I realise this solution has got an issue that it
> leaks memory if drivers are constantly being registered or unregistered. I
> find it hard to see situations where this can occur, but it is a potential
> issue.
> 
> A second solution that does not have this problem is to move the aliasing
> to EAL, as below:

Honestly I think the status quo is OK:
We have some aliases in some PMD for some historical reason
and everybody looks OK with that. Isn't it?
Bruce Richardson Oct. 20, 2022, 8:48 a.m. UTC | #3
On Thu, Oct 20, 2022 at 10:23:27AM +0200, Thomas Monjalon wrote:
> 19/10/2022 15:20, Bruce Richardson:
> > On Wed, Oct 19, 2022 at 02:11:18PM +0100, Bruce Richardson wrote:
> > > For historical reasons, a number of net vdev drivers also add a driver
> > > alias using the "eth_" prefix. Since this is done on a per-driver basis,
> > > the use of the alias in inconsistent and is spread across multiple
> > > files. We can remove the per-driver aliases by just adding the alias
> > > automatically at the vdev bus level.
> > > 
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > ---
> > >  drivers/bus/vdev/vdev.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
> > > index f5b43f1930..bfd7ce60c1 100644
> > > --- a/drivers/bus/vdev/vdev.c
> > > +++ b/drivers/bus/vdev/vdev.c
> > > @@ -54,6 +54,12 @@ static rte_spinlock_t vdev_custom_scan_lock = RTE_SPINLOCK_INITIALIZER;
> > >  void
> > >  rte_vdev_register(struct rte_vdev_driver *driver)
> > >  {
> > > +	/* For net driver vdevs, add an automatic alias using "eth" prefix */
> > > +	if (strncmp(driver->driver.name, "net_", 4) == 0 && driver->driver.alias == NULL) {
> > > +		char *alias = strdup(driver->driver.name);
> > > +		memcpy(alias, "eth_", 4);
> > > +		driver->driver.alias = alias;
> > > +	}
> > >  	TAILQ_INSERT_TAIL(&vdev_driver_list, driver, next);
> > >  }
> > >  
> > 
> > As a self-review comment, I realise this solution has got an issue that it
> > leaks memory if drivers are constantly being registered or unregistered. I
> > find it hard to see situations where this can occur, but it is a potential
> > issue.
> > 
> > A second solution that does not have this problem is to move the aliasing
> > to EAL, as below:
> 
> Honestly I think the status quo is OK:
> We have some aliases in some PMD for some historical reason
> and everybody looks OK with that. Isn't it?
> 

Well, the inconsistency bugs me a little, but if others feel the status quo
is ok, I'm ok with that.
Ferruh Yigit Oct. 20, 2022, 11:51 a.m. UTC | #4
On 10/20/2022 9:48 AM, Bruce Richardson wrote:
> On Thu, Oct 20, 2022 at 10:23:27AM +0200, Thomas Monjalon wrote:
>> 19/10/2022 15:20, Bruce Richardson:
>>> On Wed, Oct 19, 2022 at 02:11:18PM +0100, Bruce Richardson wrote:
>>>> For historical reasons, a number of net vdev drivers also add a driver
>>>> alias using the "eth_" prefix. Since this is done on a per-driver basis,
>>>> the use of the alias in inconsistent and is spread across multiple
>>>> files. We can remove the per-driver aliases by just adding the alias
>>>> automatically at the vdev bus level.
>>>>
>>>> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>>>> ---
>>>>   drivers/bus/vdev/vdev.c | 6 ++++++
>>>>   1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
>>>> index f5b43f1930..bfd7ce60c1 100644
>>>> --- a/drivers/bus/vdev/vdev.c
>>>> +++ b/drivers/bus/vdev/vdev.c
>>>> @@ -54,6 +54,12 @@ static rte_spinlock_t vdev_custom_scan_lock = RTE_SPINLOCK_INITIALIZER;
>>>>   void
>>>>   rte_vdev_register(struct rte_vdev_driver *driver)
>>>>   {
>>>> +	/* For net driver vdevs, add an automatic alias using "eth" prefix */
>>>> +	if (strncmp(driver->driver.name, "net_", 4) == 0 && driver->driver.alias == NULL) {
>>>> +		char *alias = strdup(driver->driver.name);
>>>> +		memcpy(alias, "eth_", 4);
>>>> +		driver->driver.alias = alias;
>>>> +	}
>>>>   	TAILQ_INSERT_TAIL(&vdev_driver_list, driver, next);
>>>>   }
>>>>   
>>>
>>> As a self-review comment, I realise this solution has got an issue that it
>>> leaks memory if drivers are constantly being registered or unregistered. I
>>> find it hard to see situations where this can occur, but it is a potential
>>> issue.
>>>
>>> A second solution that does not have this problem is to move the aliasing
>>> to EAL, as below:
>>
>> Honestly I think the status quo is OK:
>> We have some aliases in some PMD for some historical reason
>> and everybody looks OK with that. Isn't it?
>>
> 
> Well, the inconsistency bugs me a little, but if others feel the status quo
> is ok, I'm ok with that.

In my perspective this is for cleanup, and new PMDs keep adding alias 
because they are copying from existing drivers.
Except from above there is no harm to have alias.
David Marchand Oct. 27, 2022, 7:58 a.m. UTC | #5
On Thu, Oct 20, 2022 at 1:52 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> >> Honestly I think the status quo is OK:
> >> We have some aliases in some PMD for some historical reason
> >> and everybody looks OK with that. Isn't it?
> >>
> >
> > Well, the inconsistency bugs me a little, but if others feel the status quo
> > is ok, I'm ok with that.
>
> In my perspective this is for cleanup, and new PMDs keep adding alias
> because they are copying from existing drivers.
> Except from above there is no harm to have alias.

Do we have a "valid" case of adding new aliases?
I don't think it is the case, so we can warn of new aliases
introduction in checkpatches.sh.

At worse, if a valid case is identified later, checkpatches.sh is only
a warning in patchwork and maintainers will manually review this
warning.
Ferruh Yigit Oct. 27, 2022, 8:35 a.m. UTC | #6
On 10/27/2022 8:58 AM, David Marchand wrote:
> On Thu, Oct 20, 2022 at 1:52 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>>> Honestly I think the status quo is OK:
>>>> We have some aliases in some PMD for some historical reason
>>>> and everybody looks OK with that. Isn't it?
>>>>
>>>
>>> Well, the inconsistency bugs me a little, but if others feel the status quo
>>> is ok, I'm ok with that.
>>
>> In my perspective this is for cleanup, and new PMDs keep adding alias
>> because they are copying from existing drivers.
>> Except from above there is no harm to have alias.
> 
> Do we have a "valid" case of adding new aliases?
> I don't think it is the case, so we can warn of new aliases
> introduction in checkpatches.sh.
> 

I commented a few of them to drop alias.
checkpatch can be an option, but my intention was to drop old code to 
reduce noise, not to add more :)

OK to keep the alias if removing it will cause more trouble.

> At worse, if a valid case is identified later, checkpatches.sh is only
> a warning in patchwork and maintainers will manually review this
> warning.
> 
>
Bruce Richardson Jan. 12, 2023, 11:02 a.m. UTC | #7
On Wed, Oct 19, 2022 at 02:11:18PM +0100, Bruce Richardson wrote:
> For historical reasons, a number of net vdev drivers also add a driver
> alias using the "eth_" prefix. Since this is done on a per-driver basis,
> the use of the alias in inconsistent and is spread across multiple
> files. We can remove the per-driver aliases by just adding the alias
> automatically at the vdev bus level.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  drivers/bus/vdev/vdev.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
> index f5b43f1930..bfd7ce60c1 100644
> --- a/drivers/bus/vdev/vdev.c
> +++ b/drivers/bus/vdev/vdev.c
> @@ -54,6 +54,12 @@ static rte_spinlock_t vdev_custom_scan_lock = RTE_SPINLOCK_INITIALIZER;
>  void
>  rte_vdev_register(struct rte_vdev_driver *driver)
>  {
> +	/* For net driver vdevs, add an automatic alias using "eth" prefix */
> +	if (strncmp(driver->driver.name, "net_", 4) == 0 && driver->driver.alias == NULL) {
> +		char *alias = strdup(driver->driver.name);
> +		memcpy(alias, "eth_", 4);
> +		driver->driver.alias = alias;
> +	}
>  	TAILQ_INSERT_TAIL(&vdev_driver_list, driver, next);
>  }
>  
Just to close this off...

Following the discussion in the thread, it seems a fix is not needed/wanted
here, so dropping patch and marked "rejected" in patchwork.
diff mbox series

Patch

diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
index f5b43f1930..bfd7ce60c1 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -54,6 +54,12 @@  static rte_spinlock_t vdev_custom_scan_lock = RTE_SPINLOCK_INITIALIZER;
 void
 rte_vdev_register(struct rte_vdev_driver *driver)
 {
+	/* For net driver vdevs, add an automatic alias using "eth" prefix */
+	if (strncmp(driver->driver.name, "net_", 4) == 0 && driver->driver.alias == NULL) {
+		char *alias = strdup(driver->driver.name);
+		memcpy(alias, "eth_", 4);
+		driver->driver.alias = alias;
+	}
 	TAILQ_INSERT_TAIL(&vdev_driver_list, driver, next);
 }