In commit 681a67288655 ("usertools: check if module is loaded before binding"), script will exit if no driver is found in /sys/module/. However, for built-in kernel driver, /sys/module/MODULENAME only shows up if it has a version or at least one parameter. Take ixgbe for example, after kernel commit 34a2a3b83e2c ("net/intel: remove driver versions from Intel drivers"), and if ixgbe is built directly into kernel, there is no ixgbe folder in /sys/module. So the devbind script should not exit. Signed-off-by: Yongxin Liu <yongxin.liu@windriver.com> --- v2: - fix git commit description style in commit log - fix typo spelling --- usertools/dpdk-devbind.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py index 99112b7ab..f3c0d9814 100755 --- a/usertools/dpdk-devbind.py +++ b/usertools/dpdk-devbind.py @@ -530,10 +530,6 @@ def bind_all(dev_list, driver, force=False): # driver generated error - it's not a valid device ID, so all is well pass - # check if we're attempting to bind to a driver that isn't loaded - if not module_is_loaded(driver.replace('-','_')): - sys.exit("Error: Driver '%s' is not loaded." % driver) - try: dev_list = map(dev_id_from_dev_name, dev_list) except ValueError as ex: -- 2.14.4
On 18-Nov-20 2:58 AM, Yongxin Liu wrote: > In commit 681a67288655 ("usertools: check if module is loaded before > binding"), script will exit if no driver is found in /sys/module/. > > However, for built-in kernel driver, /sys/module/MODULENAME only > shows up if it has a version or at least one parameter. Take ixgbe > for example, after kernel commit 34a2a3b83e2c ("net/intel: remove > driver versions from Intel drivers"), and if ixgbe is built directly > into kernel, there is no ixgbe folder in /sys/module. So the devbind > script should not exit. > > Signed-off-by: Yongxin Liu <yongxin.liu@windriver.com> > --- > > v2: > - fix git commit description style in commit log > - fix typo spelling > > --- > usertools/dpdk-devbind.py | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py > index 99112b7ab..f3c0d9814 100755 > --- a/usertools/dpdk-devbind.py > +++ b/usertools/dpdk-devbind.py > @@ -530,10 +530,6 @@ def bind_all(dev_list, driver, force=False): > # driver generated error - it's not a valid device ID, so all is well > pass > > - # check if we're attempting to bind to a driver that isn't loaded > - if not module_is_loaded(driver.replace('-','_')): > - sys.exit("Error: Driver '%s' is not loaded." % driver) > - I believe there is a way to check if module is built-in, can't we use that? We could keep a list of built-in modules of interest that we can get from: cat /lib/modules/$(uname -r)/modules.builtin It's a bit more changes, but this is better than just removing the error check. > try: > dev_list = map(dev_id_from_dev_name, dev_list) > except ValueError as ex: > -- Thanks, Anatoly
A driver can be loaded as a dynamic module or a built-in module. In commit 681a67288655 ("usertools: check if module is loaded before binding"), script only checks modules in /sys/module/. However, for built-in kernel driver, it only shows up in /sys/module/, if it has a version or at least one parameter. So add check for modules in /lib/modules/$(uname -r)/modules.builtin. Thanks for Anatoly Burakov's advice. Signed-off-by: Yongxin Liu <yongxin.liu@windriver.com> --- v3: - Add built-in module list in loaded_modules for checking instead of removing error check. v2: - fix git commit description style in commit log - fix typo spelling --- usertools/dpdk-devbind.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py index 99112b7ab..5b34ccd2a 100755 --- a/usertools/dpdk-devbind.py +++ b/usertools/dpdk-devbind.py @@ -181,7 +181,13 @@ def module_is_loaded(module): loaded_modules = sysfs_mods - return module in sysfs_mods + # add built-in modules as loaded + builtin_mods = subprocess.check_output(["cat /lib/modules/$(uname -r)/modules.builtin"], shell=True).splitlines() + for mod in builtin_mods: + mod_name = os.path.basename(mod.decode("utf8")).split(".ko", 1) + loaded_modules.append(mod_name[0]) + + return module in loaded_modules def check_modules(): -- 2.14.4
> -----Original Message----- > From: Burakov, Anatoly <anatoly.burakov@intel.com> > Sent: Thursday, November 19, 2020 00:28 > To: Liu, Yongxin <Yongxin.Liu@windriver.com>; dev@dpdk.org; > thomas@monjalon.net > Subject: Re: [dpdk-dev] [PATCH v2] usertools/devbind: fix binding for > built-in 1kernel drivers > > > On 18-Nov-20 2:58 AM, Yongxin Liu wrote: > > In commit 681a67288655 ("usertools: check if module is loaded before > > binding"), script will exit if no driver is found in /sys/module/. > > > > However, for built-in kernel driver, /sys/module/MODULENAME only shows > > up if it has a version or at least one parameter. Take ixgbe for > > example, after kernel commit 34a2a3b83e2c ("net/intel: remove driver > > versions from Intel drivers"), and if ixgbe is built directly into > > kernel, there is no ixgbe folder in /sys/module. So the devbind script > > should not exit. > > > > Signed-off-by: Yongxin Liu <yongxin.liu@windriver.com> > > --- > > > > v2: > > - fix git commit description style in commit log > > - fix typo spelling > > > > --- > > usertools/dpdk-devbind.py | 4 ---- > > 1 file changed, 4 deletions(-) > > > > diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py > > index 99112b7ab..f3c0d9814 100755 > > --- a/usertools/dpdk-devbind.py > > +++ b/usertools/dpdk-devbind.py > > @@ -530,10 +530,6 @@ def bind_all(dev_list, driver, force=False): > > # driver generated error - it's not a valid device ID, so all > is well > > pass > > > > - # check if we're attempting to bind to a driver that isn't loaded > > - if not module_is_loaded(driver.replace('-','_')): > > - sys.exit("Error: Driver '%s' is not loaded." % driver) > > - > > I believe there is a way to check if module is built-in, can't we use that? > We could keep a list of built-in modules of interest that we can get from: > > cat /lib/modules/$(uname -r)/modules.builtin > > It's a bit more changes, but this is better than just removing the error > check. Thanks Anatoly for your advice. I have sent v3. Please review. /Yongxin > > > try: > > dev_list = map(dev_id_from_dev_name, dev_list) > > except ValueError as ex: > > > > > -- > Thanks, > Anatoly
On 19-Nov-20 7:16 AM, Yongxin Liu wrote: > A driver can be loaded as a dynamic module or a built-in module. > In commit 681a67288655 ("usertools: check if module is loaded > before binding"), script only checks modules in /sys/module/. > > However, for built-in kernel driver, it only shows up in /sys/module/, > if it has a version or at least one parameter. So add check for > modules in /lib/modules/$(uname -r)/modules.builtin. > > Thanks for Anatoly Burakov's advice. > > Signed-off-by: Yongxin Liu <yongxin.liu@windriver.com> > --- > > v3: > - Add built-in module list in loaded_modules for checking > instead of removing error check. > > v2: > - fix git commit description style in commit log > - fix typo spelling > > --- > usertools/dpdk-devbind.py | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py > index 99112b7ab..5b34ccd2a 100755 > --- a/usertools/dpdk-devbind.py > +++ b/usertools/dpdk-devbind.py > @@ -181,7 +181,13 @@ def module_is_loaded(module): > > loaded_modules = sysfs_mods > > - return module in sysfs_mods > + # add built-in modules as loaded > + builtin_mods = subprocess.check_output(["cat /lib/modules/$(uname -r)/modules.builtin"], shell=True).splitlines() I'd rather not call shell directly, it's bad practice and is potentially unsafe. The $(uname -r) can be replaced with: import platform release = platform.uname().release # equivalent to $(uname -r) The rest can follow: filename = os.path.join("/lib/modules/", release, "modules.builtin") # read the file contents, split lines, etc. Also, please check if file exists to ensure we don't crash the script. > + for mod in builtin_mods: > + mod_name = os.path.basename(mod.decode("utf8")).split(".ko", 1) os.path.splitext(os.path.basename(...))? > + loaded_modules.append(mod_name[0]) > + > + return module in loaded_modules > > > def check_modules(): > -- Thanks, Anatoly
A driver can be loaded as a dynamic module or a built-in module. In commit 681a67288655 ("usertools: check if module is loaded before binding"), script only checks modules in /sys/module/. However, for built-in kernel driver, it only shows up in /sys/module/, if it has a version or at least one parameter. So add check for modules in /lib/modules/$(uname -r)/modules.builtin. Thanks for Anatoly Burakov's advice. Signed-off-by: Yongxin Liu <yongxin.liu@windriver.com> --- v4: - Replace shell call with platform.uname(). Check file existence before reading. v3: - Add built-in module list in loaded_modules for checking instead of removing error check. v2: - fix git commit description style in commit log - fix typo spelling --- usertools/dpdk-devbind.py | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py index 99112b7ab..06721709c 100755 --- a/usertools/dpdk-devbind.py +++ b/usertools/dpdk-devbind.py @@ -7,6 +7,7 @@ import os import getopt import subprocess +import platform from glob import glob from os.path import exists, abspath, dirname, basename from os.path import join as path_join @@ -181,7 +182,23 @@ def module_is_loaded(module): loaded_modules = sysfs_mods - return module in sysfs_mods + # add built-in modules as loaded + release = platform.uname().release + filename = os.path.join("/lib/modules/", release, "modules.builtin") + if os.path.exists(filename): + try: + f = open(filename, "r") + except: + print("Error: cannot open %s" % filename) + return + + builtin_mods = f.readlines() + + for mod in builtin_mods: + mod_name = os.path.splitext(os.path.basename(mod)) + loaded_modules.append(mod_name[0]) + + return module in loaded_modules def check_modules(): -- 2.14.4
> -----Original Message----- > From: Burakov, Anatoly <anatoly.burakov@intel.com> > Sent: Thursday, November 19, 2020 20:19 > To: Liu, Yongxin <Yongxin.Liu@windriver.com>; dev@dpdk.org; > thomas@monjalon.net > Subject: Re: [dpdk-dev] [PATCH v3] usertools/devbind: fix binding for > built-in kernel drivers > > > On 19-Nov-20 7:16 AM, Yongxin Liu wrote: > > A driver can be loaded as a dynamic module or a built-in module. > > In commit 681a67288655 ("usertools: check if module is loaded before > > binding"), script only checks modules in /sys/module/. > > > > However, for built-in kernel driver, it only shows up in /sys/module/, > > if it has a version or at least one parameter. So add check for > > modules in /lib/modules/$(uname -r)/modules.builtin. > > > > Thanks for Anatoly Burakov's advice. > > > > Signed-off-by: Yongxin Liu <yongxin.liu@windriver.com> > > --- > > > > v3: > > - Add built-in module list in loaded_modules for checking > > instead of removing error check. > > > > v2: > > - fix git commit description style in commit log > > - fix typo spelling > > > > --- > > usertools/dpdk-devbind.py | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py > > index 99112b7ab..5b34ccd2a 100755 > > --- a/usertools/dpdk-devbind.py > > +++ b/usertools/dpdk-devbind.py > > @@ -181,7 +181,13 @@ def module_is_loaded(module): > > > > loaded_modules = sysfs_mods > > > > - return module in sysfs_mods > > + # add built-in modules as loaded > > + builtin_mods = subprocess.check_output(["cat /lib/modules/$(uname > > + -r)/modules.builtin"], shell=True).splitlines() > > I'd rather not call shell directly, it's bad practice and is potentially > unsafe. The $(uname -r) can be replaced with: > > import platform > release = platform.uname().release # equivalent to $(uname -r) > > The rest can follow: > > filename = os.path.join("/lib/modules/", release, > "modules.builtin") > # read the file contents, split lines, etc. > > Also, please check if file exists to ensure we don't crash the script. > > > + for mod in builtin_mods: > > + mod_name = os.path.basename(mod.decode("utf8")).split(".ko", > > + 1) > > os.path.splitext(os.path.basename(...))? Thanks Anatoly for your detailed instruction. v4 has been sent. /Yongxin > > > + loaded_modules.append(mod_name[0]) > > + > > + return module in loaded_modules > > > > > > def check_modules(): > > > > > -- > Thanks, > Anatoly
On 20-Nov-20 2:22 AM, Yongxin Liu wrote: > A driver can be loaded as a dynamic module or a built-in module. > In commit 681a67288655 ("usertools: check if module is loaded > before binding"), script only checks modules in /sys/module/. > > However, for built-in kernel driver, it only shows up in /sys/module/, > if it has a version or at least one parameter. So add check for > modules in /lib/modules/$(uname -r)/modules.builtin. > > Thanks for Anatoly Burakov's advice. > > Signed-off-by: Yongxin Liu <yongxin.liu@windriver.com> > --- > > v4: > - Replace shell call with platform.uname(). Check file existence > before reading. > > v3: > - Add built-in module list in loaded_modules for checking > instead of removing error check. > > v2: > - fix git commit description style in commit log > - fix typo spelling > > --- > usertools/dpdk-devbind.py | 19 ++++++++++++++++++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py > index 99112b7ab..06721709c 100755 > --- a/usertools/dpdk-devbind.py > +++ b/usertools/dpdk-devbind.py > @@ -7,6 +7,7 @@ > import os > import getopt > import subprocess > +import platform > from glob import glob > from os.path import exists, abspath, dirname, basename > from os.path import join as path_join > @@ -181,7 +182,23 @@ def module_is_loaded(module): > > loaded_modules = sysfs_mods > > - return module in sysfs_mods > + # add built-in modules as loaded > + release = platform.uname().release > + filename = os.path.join("/lib/modules/", release, "modules.builtin") > + if os.path.exists(filename): > + try: > + f = open(filename, "r") > + except: > + print("Error: cannot open %s" % filename) > + return > + > + builtin_mods = f.readlines() > + > + for mod in builtin_mods: > + mod_name = os.path.splitext(os.path.basename(mod)) > + loaded_modules.append(mod_name[0]) > + You're not returning a value in error case, this would cause error in the caller of this function. Also, i'd avoid reading the entire file into memory, instead I'd do something like this: try: with open(filename) as f: loaded_modules += [os.path.splitext(os.path.basename(mod)[0] for mod in f] except IOError: print("Warning: cannot read list of built-in kernel modules") # continue with regular code This will be faster, more and readable as well :) > + return module in loaded_modules > > > def check_modules(): > -- Thanks, Anatoly
On 20-Nov-20 2:22 AM, Yongxin Liu wrote:
> A driver can be loaded as a dynamic module or a built-in module.
> In commit 681a67288655 ("usertools: check if module is loaded
> before binding"), script only checks modules in /sys/module/.
>
> However, for built-in kernel driver, it only shows up in /sys/module/,
> if it has a version or at least one parameter. So add check for
> modules in /lib/modules/$(uname -r)/modules.builtin.
>
> Thanks for Anatoly Burakov's advice.
Also, no need to add this last line to the commit log :)
--
Thanks,
Anatoly
A driver can be loaded as a dynamic module or a built-in module. In commit 681a67288655 ("usertools: check if module is loaded before binding"), script only checks modules in /sys/module/. However, for built-in kernel driver, it only shows up in /sys/module/, if it has a version or at least one parameter. So add check for modules in /lib/modules/$(uname -r)/modules.builtin. Signed-off-by: Yongxin Liu <yongxin.liu@windriver.com> --- v5: - Make code robust and more memory efficient. v4: - Replace shell call with platform.uname(). Check file existence before reading. v3: - Add built-in module list in loaded_modules for checking instead of removing error check. v2: - fix git commit description style in commit log - fix typo spelling --- usertools/dpdk-devbind.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py index 054ad2e1c..4bc0b6207 100755 --- a/usertools/dpdk-devbind.py +++ b/usertools/dpdk-devbind.py @@ -7,6 +7,7 @@ import os import subprocess import argparse +import platform from glob import glob from os.path import exists, basename @@ -107,7 +108,17 @@ def module_is_loaded(module): loaded_modules = sysfs_mods - return module in sysfs_mods + # add built-in modules as loaded + release = platform.uname().release + filename = os.path.join("/lib/modules/", release, "modules.builtin") + if os.path.exists(filename): + try: + with open(filename) as f: + loaded_modules += [os.path.splitext(os.path.basename(mod))[0] for mod in f] + except IOError: + print("Warning: cannot read list of built-in kernel modules") + + return module in loaded_modules def check_modules(): -- 2.14.4
Hi Anatoly,
Do you have any further comments on this v5?
Or you can submit your own patch directly.
I am really expecting this issue to be fixed.
Thank you very much.
Yongxin
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Yongxin Liu
> Sent: Monday, November 23, 2020 11:06
> To: dev@dpdk.org; anatoly.burakov@intel.com; thomas@monjalon.net
> Subject: [dpdk-dev] [PATCH v5] usertools/devbind: fix binding for built-in
> kernel drivers
>
> A driver can be loaded as a dynamic module or a built-in module.
> In commit 681a67288655 ("usertools: check if module is loaded before
> binding"), script only checks modules in /sys/module/.
>
> However, for built-in kernel driver, it only shows up in /sys/module/, if
> it has a version or at least one parameter. So add check for modules in
> /lib/modules/$(uname -r)/modules.builtin.
>
> Signed-off-by: Yongxin Liu <yongxin.liu@windriver.com>
> ---
>
> v5:
> - Make code robust and more memory efficient.
>
> v4:
> - Replace shell call with platform.uname(). Check file existence
> before reading.
>
> v3:
> - Add built-in module list in loaded_modules for checking
> instead of removing error check.
>
> v2:
> - fix git commit description style in commit log
> - fix typo spelling
>
> ---
> usertools/dpdk-devbind.py | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py index
> 054ad2e1c..4bc0b6207 100755
> --- a/usertools/dpdk-devbind.py
> +++ b/usertools/dpdk-devbind.py
> @@ -7,6 +7,7 @@
> import os
> import subprocess
> import argparse
> +import platform
>
> from glob import glob
> from os.path import exists, basename
> @@ -107,7 +108,17 @@ def module_is_loaded(module):
>
> loaded_modules = sysfs_mods
>
> - return module in sysfs_mods
> + # add built-in modules as loaded
> + release = platform.uname().release
> + filename = os.path.join("/lib/modules/", release, "modules.builtin")
> + if os.path.exists(filename):
> + try:
> + with open(filename) as f:
> + loaded_modules +=
> [os.path.splitext(os.path.basename(mod))[0] for mod in f]
> + except IOError:
> + print("Warning: cannot read list of built-in kernel
> + modules")
> +
> + return module in loaded_modules
>
>
> def check_modules():
> --
> 2.14.4
On 03-Dec-20 8:25 AM, Liu, Yongxin wrote:
> Hi Anatoly,
>
> Do you have any further comments on this v5?
> Or you can submit your own patch directly.
>
> I am really expecting this issue to be fixed.
>
>
> Thank you very much.
>
> Yongxin
>
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Yongxin Liu
>> Sent: Monday, November 23, 2020 11:06
>> To: dev@dpdk.org; anatoly.burakov@intel.com; thomas@monjalon.net
>> Subject: [dpdk-dev] [PATCH v5] usertools/devbind: fix binding for built-in
>> kernel drivers
>>
>> A driver can be loaded as a dynamic module or a built-in module.
>> In commit 681a67288655 ("usertools: check if module is loaded before
>> binding"), script only checks modules in /sys/module/.
>>
>> However, for built-in kernel driver, it only shows up in /sys/module/, if
>> it has a version or at least one parameter. So add check for modules in
>> /lib/modules/$(uname -r)/modules.builtin.
>>
>> Signed-off-by: Yongxin Liu <yongxin.liu@windriver.com>
>> ---
>>
>> v5:
>> - Make code robust and more memory efficient.
>>
>> v4:
>> - Replace shell call with platform.uname(). Check file existence
>> before reading.
>>
>> v3:
>> - Add built-in module list in loaded_modules for checking
>> instead of removing error check.
>>
>> v2:
>> - fix git commit description style in commit log
>> - fix typo spelling
>>
>> ---
Apologies for the delay.
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
--
Thanks,
Anatoly
> >> A driver can be loaded as a dynamic module or a built-in module.
> >> In commit 681a67288655 ("usertools: check if module is loaded before
> >> binding"), script only checks modules in /sys/module/.
> >>
> >> However, for built-in kernel driver, it only shows up in /sys/module/, if
> >> it has a version or at least one parameter. So add check for modules in
> >> /lib/modules/$(uname -r)/modules.builtin.
> >>
> >> Signed-off-by: Yongxin Liu <yongxin.liu@windriver.com>
>
> Apologies for the delay.
>
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
Fixes: 681a67288655 ("usertools: check if module is loaded before binding")
Cc: stable@dpdk.org
Applied, thanks