Re: [dpdk-dev] [PATCH] rte, eal: Rename pci_update_device and make it public
Hi Thomas, That's OK. Hope that the need of usage can be considered in the new framework. -Original Message- From: Thomas Monjalon [mailto:thomas.monja...@6wind.com] Sent: Friday, March 10, 2017 1:58 AM To: Yang, Ziye Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH] rte, eal: Rename pci_update_device and make it public 2017-02-13 10:53, Ziye Yang: > The reaon is that sometimes we only like to rebound the kernel driver > or VFIO or UIO or other drivers for this device after rte_eal_detach > function. Function rte_eal_pci_probe_one not only updates the device > but also probes the rte_eal_driver for this device, it is not > flexible. > > Signed-off-by: Ziye Yang > --- > lib/librte_eal/bsdapp/eal/eal_pci.c | 2 +- > lib/librte_eal/common/eal_common_pci.c | 2 +- > lib/librte_eal/common/eal_private.h | 13 - > lib/librte_eal/common/include/rte_pci.h | 14 ++ > lib/librte_eal/linuxapp/eal/eal_pci.c | 2 +- > 5 files changed, 17 insertions(+), 16 deletions(-) The PCI bus management is going to change with the new bus framework. Please check pending developments or future version for your needs. This patch will be marked as rejected because of the conflicting work in progress.
Re: [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak issue of logid
Hi Konstantin,
The global logid is used to prevent the memory leak. If we do not do this, but
directly free(logid), this pointer will still be used by openlog, which means
that we cannot free the logid anymore. And by adding the global variable, it
will not only solve the memory leak but also makes the openlog function still
work.
Best Regards
Ziye Yang
-Original Message-
From: Ananyev, Konstantin
Sent: Monday, September 10, 2018 7:55 PM
To: Yang, Ziye ; dev@dpdk.org
Cc: Ziye Yang
Subject: RE: [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak issue of
logid
Hi
> -Original Message-
> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Ziye Yang
> Sent: Wednesday, September 5, 2018 6:39 AM
> To: dev@dpdk.org
> Cc: Ziye Yang
> Subject: [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak
> issue of logid
>
> From: Ziye Yang
>
> This patch is used to fix the memory leak issue of logid.
> We use the ASAN test in SPDK when intergrating DPDK and find this
> memory leak issue.
>
> Signed-off-by: Ziye Yang
> ---
> lib/librte_eal/linuxapp/eal/eal.c | 21 -
> lib/librte_eal/linuxapp/eal/eal_log.c | 11 ++-
> 2 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c
> b/lib/librte_eal/linuxapp/eal/eal.c
> index e59ac65..e150200 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -793,7 +793,7 @@ static void rte_eal_init_alert(const char *msg)
> int i, fctret, ret;
> pthread_t thread_id;
> static rte_atomic32_t run_once = RTE_ATOMIC32_INIT(0);
> - const char *logid;
> + char *logid;
> char cpuset[RTE_CPU_AFFINITY_STR_LEN];
> char thread_name[RTE_MAX_THREAD_NAME_LEN];
>
> @@ -812,6 +812,12 @@ static void rte_eal_init_alert(const char *msg)
>
> logid = strrchr(argv[0], '/');
> logid = strdup(logid ? logid + 1: argv[0]);
> + if (!logid) {
> + rte_eal_init_alert("Cannot allocate memory for logid\n");
> + rte_errno = ENOMEM;
> + rte_atomic32_clear(&run_once);
> + return -1;
> + }
>
> thread_id = pthread_self();
>
> @@ -823,6 +829,8 @@ static void rte_eal_init_alert(const char *msg)
> if (rte_eal_cpu_init() < 0) {
> rte_eal_init_alert("Cannot detect lcores.");
> rte_errno = ENOTSUP;
> + rte_atomic32_clear(&run_once);
> + free(logid);
> return -1;
> }
>
> @@ -831,6 +839,7 @@ static void rte_eal_init_alert(const char *msg)
> rte_eal_init_alert("Invalid 'command line' arguments.");
> rte_errno = EINVAL;
> rte_atomic32_clear(&run_once);
> + free(logid);
> return -1;
> }
>
> @@ -838,12 +847,14 @@ static void rte_eal_init_alert(const char *msg)
> rte_eal_init_alert("Cannot init plugins\n");
> rte_errno = EINVAL;
> rte_atomic32_clear(&run_once);
> + free(logid);
> return -1;
> }
>
> if (eal_option_device_parse()) {
> rte_errno = ENODEV;
> rte_atomic32_clear(&run_once);
> + free(logid);
> return -1;
> }
>
> @@ -851,6 +862,8 @@ static void rte_eal_init_alert(const char *msg)
>
> if (rte_eal_intr_init() < 0) {
> rte_eal_init_alert("Cannot init interrupt-handling thread\n");
> + rte_atomic32_clear(&run_once);
> + free(logid);
> return -1;
> }
>
> @@ -861,6 +874,8 @@ static void rte_eal_init_alert(const char *msg)
> rte_eal_init_alert("failed to init mp channel\n");
> if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> rte_errno = EFAULT;
> + rte_atomic32_clear(&run_once);
> + free(logid);
> return -1;
> }
> }
> @@ -869,6 +884,7 @@ static void rte_eal_init_alert(const char *msg)
> rte_eal_init_alert("Cannot scan the buses for devices\n");
> rte_errno = ENODEV;
> rte_atomic32_clear(&run_once);
> + free(logid);
> return -1;
> }
>
> @@ -893,6 +909,7 @@ static void rte_eal_init_alert(const char *msg)
> rte_eal_init_alert("Cannot get hugepage information.");
> rte_errno = EACCES;
>
Re: [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak issue of logid
Hi Konstantin,
Thanks for your comments. I will revise the patch and submit another patch
later.
Best Regards
Ziye Yang
-Original Message-
From: Ananyev, Konstantin
Sent: Monday, September 10, 2018 8:19 PM
To: Yang, Ziye ; dev@dpdk.org
Cc: Ziye Yang
Subject: RE: [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak issue of
logid
> -Original Message-
> From: Yang, Ziye
> Sent: Monday, September 10, 2018 12:58 PM
> To: Ananyev, Konstantin ; dev@dpdk.org
> Cc: Ziye Yang
> Subject: RE: [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak
> issue of logid
>
> Hi Konstantin,
>
> The global logid is used to prevent the memory leak. If we do not do
> this, but directly free(logid), this pointer will still be used by
> openlog, which means that we cannot free the logid anymore. And by adding the
> global variable, it will not only solve the memory leak but also makes the
> openlog function still work.
But then I suppose you can avoid any dynamic memory allocation at all with
something like that:
rte_eal_init(int argc, char **argv)
{
...
static char logid[PATH_MAX];
...
const char *p= strrchr(argv[0], '/');
snprintf(logid, sizeof(logid), "%s", p);
...
rte_eal_log_init(logid, ...);
?
Konstantin
>
>
> Best Regards
> Ziye Yang
>
> -Original Message-
> From: Ananyev, Konstantin
> Sent: Monday, September 10, 2018 7:55 PM
> To: Yang, Ziye ; dev@dpdk.org
> Cc: Ziye Yang
> Subject: RE: [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak
> issue of logid
>
> Hi
>
> > -Original Message-
> > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Ziye Yang
> > Sent: Wednesday, September 5, 2018 6:39 AM
> > To: dev@dpdk.org
> > Cc: Ziye Yang
> > Subject: [dpdk-dev] [PATCH v5] linuxapp, eal: Fix the memory leak
> > issue of logid
> >
> > From: Ziye Yang
> >
> > This patch is used to fix the memory leak issue of logid.
> > We use the ASAN test in SPDK when intergrating DPDK and find this
> > memory leak issue.
> >
> > Signed-off-by: Ziye Yang
> > ---
> > lib/librte_eal/linuxapp/eal/eal.c | 21 -
> > lib/librte_eal/linuxapp/eal/eal_log.c | 11 ++-
> > 2 files changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_eal/linuxapp/eal/eal.c
> > b/lib/librte_eal/linuxapp/eal/eal.c
> > index e59ac65..e150200 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal.c
> > @@ -793,7 +793,7 @@ static void rte_eal_init_alert(const char *msg)
> > int i, fctret, ret;
> > pthread_t thread_id;
> > static rte_atomic32_t run_once = RTE_ATOMIC32_INIT(0);
> > - const char *logid;
> > + char *logid;
> > char cpuset[RTE_CPU_AFFINITY_STR_LEN];
> > char thread_name[RTE_MAX_THREAD_NAME_LEN];
> >
> > @@ -812,6 +812,12 @@ static void rte_eal_init_alert(const char *msg)
> >
> > logid = strrchr(argv[0], '/');
> > logid = strdup(logid ? logid + 1: argv[0]);
> > + if (!logid) {
> > + rte_eal_init_alert("Cannot allocate memory for logid\n");
> > + rte_errno = ENOMEM;
> > + rte_atomic32_clear(&run_once);
> > + return -1;
> > + }
> >
> > thread_id = pthread_self();
> >
> > @@ -823,6 +829,8 @@ static void rte_eal_init_alert(const char *msg)
> > if (rte_eal_cpu_init() < 0) {
> > rte_eal_init_alert("Cannot detect lcores.");
> > rte_errno = ENOTSUP;
> > + rte_atomic32_clear(&run_once);
> > + free(logid);
> > return -1;
> > }
> >
> > @@ -831,6 +839,7 @@ static void rte_eal_init_alert(const char *msg)
> > rte_eal_init_alert("Invalid 'command line' arguments.");
> > rte_errno = EINVAL;
> > rte_atomic32_clear(&run_once);
> > + free(logid);
> > return -1;
> > }
> >
> > @@ -838,12 +847,14 @@ static void rte_eal_init_alert(const char *msg)
> > rte_eal_init_alert("Cannot init plugins\n");
> > rte_errno = EINVAL;
> > rte_atomic32_clear(&run_once);
> > + free(logid);
> > return -1;
> > }
> >
> > if (eal_option_device_parse()) {
> > rte_errno = ENODEV;
> > rte_atomic32_clear(&run_once);
> > + free(logid);
> > return -1;
> >
[dpdk-dev] [PATCH v2] PCI: ABI change request for adding new field in rte_pci_id structure
Hi Panu, " ABI breakage announcements go into doc/guides/rel_notes/deprecation.rst, see the examples there. Also you can't break the ABI in the version under development but only the next one, so right now the earliest ABI breakage opportunity is in the release *after* 16.04, which is supposed to be 16.07 according to the roadmap." So I only need to do the ABI breakage announcements into doc/guides/rel_notes/deprecation.rst, right? Thanks. Best Regards, Ziye Yang -Original Message- From: Panu Matilainen [mailto:pmati...@redhat.com] Sent: Tuesday, February 16, 2016 3:39 PM To: Yang, Ziye ; dev at dpdk.org Subject: Re: [dpdk-dev] [PATCH v2] PCI: ABI change request for adding new field in rte_pci_id structure On 02/16/2016 05:16 AM, Ziye Yang wrote: > From: Ziye > > The purpose of this patch is used to add a new field "class" in > rte_pci_id structure. The new class field includes class_id, > subcalss_id, programming interface of a pci device. > With this field, we can identify pci device by its class info, which > can be more flexible instead of probing the device by vendor_id OR > device_id OR subvendor_id OR subdevice_id. > For example, we can probe all nvme devices by class field, which can > be quite convenient. > > Signed-off-by: Ziye Yang > --- > doc/guides/rel_notes/release_16_04.rst | 7 --- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/doc/guides/rel_notes/release_16_04.rst > b/doc/guides/rel_notes/release_16_04.rst > index 27fc624..fe843a5 100644 > --- a/doc/guides/rel_notes/release_16_04.rst > +++ b/doc/guides/rel_notes/release_16_04.rst > @@ -95,9 +95,10 @@ This section should contain API changes. Sample format: > ABI Changes > --- > > -* Add a short 1-2 sentence description of the ABI change that was > announced in > - the previous releases and made in this release. Use fixed width > quotes for > - ``rte_function_names`` or ``rte_struct_names``. Use the past tense. > +* New field ``class`` is added into ``rte_pci_id`` structure. This > +new > + added ``class`` field can be used to probe pci devices by class > +related > + info. With this new field, the size of structure ``rte_pci_device`` > +will > + be increased. > > > Shared Library Versions > ABI breakage announcements go into doc/guides/rel_notes/deprecation.rst, see the examples there. Also you can't break the ABI in the version under development but only the next one, so right now the earliest ABI breakage opportunity is in the release *after* 16.04, which is supposed to be 16.07 according to the roadmap. - Panu -
[dpdk-dev] [PATCH] librte_eal: fix wrong args operation in eal_parse_args
Hi Bruce, Could it be fixed later? If not, it should be documented. I faced this issued today, and found that dpdk changed my last arg. In my mind, dpdk should not change the argv[last], which will confuse the users. Best Regards, Ziye Yang -Original Message- From: Richardson, Bruce Sent: Wednesday, May 11, 2016 7:25 PM To: Yang, Ziye Cc: dev at dpdk.org Subject: Re: [dpdk-dev] [PATCH] librte_eal: fix wrong args operation in eal_parse_args On Wed, May 11, 2016 at 01:28:21PM +0800, Ziye Yang wrote: > This patch is used to fix wrong operation on user input args. > eal_parse_args function should not operate the args passed by the > user. If the element in argv is generated by malloc function, changing > it will cause memory issues when free the args. > > Signed-off-by: Ziye Yang > --- > lib/librte_eal/bsdapp/eal/eal.c | 2 -- > lib/librte_eal/linuxapp/eal/eal.c | 2 -- > 2 files changed, 4 deletions(-) > > diff --git a/lib/librte_eal/bsdapp/eal/eal.c > b/lib/librte_eal/bsdapp/eal/eal.c index 06bfd4e..0eef92d 100644 > --- a/lib/librte_eal/bsdapp/eal/eal.c > +++ b/lib/librte_eal/bsdapp/eal/eal.c > @@ -420,8 +420,6 @@ eal_parse_args(int argc, char **argv) > goto out; > } > > - if (optind >= 0) > - argv[optind-1] = prgname; > ret = optind-1; > > out: > diff --git a/lib/librte_eal/linuxapp/eal/eal.c > b/lib/librte_eal/linuxapp/eal/eal.c > index 8aafd51..ba9d1ac 100644 > --- a/lib/librte_eal/linuxapp/eal/eal.c > +++ b/lib/librte_eal/linuxapp/eal/eal.c > @@ -658,8 +658,6 @@ eal_parse_args(int argc, char **argv) > goto out; > } > > - if (optind >= 0) > - argv[optind-1] = prgname; > ret = optind-1; > > out: This is a behaviour change in DPDK. The behaviour has always been that after calling eal init, you can update your argv/argc values by the number of args parsed and then parse your app args as normal, since argv[0] will still point to your program name. While I agree that having the current behaviour may cause some problems, changing this behaviour may break applications that have been written to use the existing behaviour. Since it is only the last EAL parameter arg that is modified, I think it would be acceptable to have the behaviour well documented and then expect any app to store a second copy of the pointer to be modified if it is needed for a subsequent free call, for example. /Bruce
[dpdk-dev] [PATCH] librte_eal: fix wrong args operation in eal_parse_args
Hi Bruce, Could you tell me the documentation file name? Then I can conduct the following documentation relate d patch. -Original Message- From: Richardson, Bruce Sent: Wednesday, May 11, 2016 8:21 PM To: Yang, Ziye Cc: dev at dpdk.org Subject: RE: [dpdk-dev] [PATCH] librte_eal: fix wrong args operation in eal_parse_args > -Original Message- > From: Yang, Ziye > Sent: Wednesday, May 11, 2016 12:51 PM > To: Richardson, Bruce > Cc: dev at dpdk.org > Subject: RE: [dpdk-dev] [PATCH] librte_eal: fix wrong args operation > in eal_parse_args > > Hi Bruce, > > Could it be fixed later? If not, it should be documented. I faced > this issued today, and found that dpdk changed my last arg. In my > mind, dpdk should not change the argv[last], which will confuse the users. > We can certainly consider changing it, but I am concerned about stability of existing apps. I think it needs some discussion and consensus on-list before a change like this is made. For right now, definitely the behavior should be documented. Would you consider submitting a documentation update patch for this issue instead of this code change? Thanks, /Bruce > Best Regards, > Ziye Yang > > -Original Message- > From: Richardson, Bruce > Sent: Wednesday, May 11, 2016 7:25 PM > To: Yang, Ziye > Cc: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH] librte_eal: fix wrong args operation > in eal_parse_args > > On Wed, May 11, 2016 at 01:28:21PM +0800, Ziye Yang wrote: > > This patch is used to fix wrong operation on user input args. > > eal_parse_args function should not operate the args passed by the > > user. If the element in argv is generated by malloc function, > > changing it will cause memory issues when free the args. > > > > Signed-off-by: Ziye Yang > > --- > > lib/librte_eal/bsdapp/eal/eal.c | 2 -- > > lib/librte_eal/linuxapp/eal/eal.c | 2 -- > > 2 files changed, 4 deletions(-) > > > > diff --git a/lib/librte_eal/bsdapp/eal/eal.c > > b/lib/librte_eal/bsdapp/eal/eal.c index 06bfd4e..0eef92d 100644 > > --- a/lib/librte_eal/bsdapp/eal/eal.c > > +++ b/lib/librte_eal/bsdapp/eal/eal.c > > @@ -420,8 +420,6 @@ eal_parse_args(int argc, char **argv) > > goto out; > > } > > > > - if (optind >= 0) > > - argv[optind-1] = prgname; > > ret = optind-1; > > > > out: > > diff --git a/lib/librte_eal/linuxapp/eal/eal.c > > b/lib/librte_eal/linuxapp/eal/eal.c > > index 8aafd51..ba9d1ac 100644 > > --- a/lib/librte_eal/linuxapp/eal/eal.c > > +++ b/lib/librte_eal/linuxapp/eal/eal.c > > @@ -658,8 +658,6 @@ eal_parse_args(int argc, char **argv) > > goto out; > > } > > > > - if (optind >= 0) > > - argv[optind-1] = prgname; > > ret = optind-1; > > > > out: > This is a behaviour change in DPDK. The behaviour has always been that > after calling eal init, you can update your argv/argc values by the > number of args parsed and then parse your app args as normal, since > argv[0] will still point to your program name. While I agree that > having the current behaviour may cause some problems, changing this > behaviour may break applications that have been written to use the existing > behaviour. > > Since it is only the last EAL parameter arg that is modified, I think > it would be acceptable to have the behaviour well documented and then > expect any app to store a second copy of the pointer to be modified if > it is needed for a subsequent free call, for example. > > /Bruce
[dpdk-dev] [PATCH] pci: Add the class_id support in pci probe
-Original Message-
From: Thomas Monjalon [mailto:thomas.monja...@6wind.com]
Sent: Thursday, May 19, 2016 6:34 PM
To: Yang, Ziye
Cc: dev at dpdk.org
Subject: Re: [dpdk-dev] [PATCH] pci: Add the class_id support in pci probe
2016-05-11 14:08, Ziye Yang:
> This patch is used to add the class_id (class_code, subclass_code,
> programming_interface) support for pci_device probe. With this patch,
> it will be flexible for users to probe a class of devices by class_id.
>
> Signed-off-by: Ziye Yang
> ---
> lib/librte_eal/bsdapp/eal/eal_pci.c | 4
> lib/librte_eal/common/eal_common_pci.c | 3 +++
> lib/librte_eal/common/include/rte_pci.h | 8 ++--
> lib/librte_eal/linuxapp/eal/eal_pci.c | 9 +
> 4 files changed, 22 insertions(+), 2 deletions(-)
Please remove the deprecation notice.
> --- a/lib/librte_eal/common/include/rte_pci.h
> +++ b/lib/librte_eal/common/include/rte_pci.h
> @@ -129,6 +129,7 @@ struct rte_pci_id {
> uint16_t device_id; /**< Device ID or PCI_ANY_ID. */
> uint16_t subsystem_vendor_id; /**< Subsystem vendor ID or PCI_ANY_ID. */
> uint16_t subsystem_device_id; /**< Subsystem device ID or
> PCI_ANY_ID. */
> + uint32_t class_id; /**< Class ID (class, subclass, pi) or
> CLASS_ANY_ID. */
> };
A space is missing.
It would be more logical to put the class_id at the beginning of the struct.
> /** Any PCI device identifier (vendor, device, ...) */ #define
> PCI_ANY_ID (0x)
> +#define CLASS_ANY_ID (0xff)
These constants should be prefixed with RTE_.
[Ziye] suggest for doing another patch to change PCI_ANY_ID to RTE_PCI_ANY_ID
since it will affect
Other files.
> --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
> + /* get class_id */
> + snprintf(filename, sizeof(filename), "%s/class",
> + dirname);
> + if (eal_parse_sysfs_value(filename, &tmp) < 0) {
> + free(dev);
> + return -1;
> + }
> + dev->id.class_id = (uint32_t)tmp && CLASS_ANY_ID;
Should be a bitwise &. Why masking is needed?
[Ziye] Only 24bit info is needed.
[dpdk-dev] [PATCH] pci: Add the class_id support in pci probe
-Original Message- From: Thomas Monjalon [mailto:thomas.monja...@6wind.com] Sent: Thursday, May 19, 2016 8:57 PM To: Yang, Ziye Cc: dev at dpdk.org Subject: Re: [dpdk-dev] [PATCH] pci: Add the class_id support in pci probe 2016-05-19 12:18, Yang, Ziye: > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > 2016-05-11 14:08, Ziye Yang: > > + dev->id.class_id = (uint32_t)tmp && CLASS_ANY_ID; > > Should be a bitwise &. Why masking is needed? > [Ziye] Only 24bit info is needed. What are the other bits? Please put a comment in the code. [Ziye] Revision ID is defined in pci spec, classid has 24 bits. And when we read from the system, we will only get class_id, subclass and program interface. I will put the comment
