[dpdk-dev] [PATCH v5 1/2] librte_ether: add protection against overwrite device data

2016-10-07 Thread Kerlin, MarcinX
Hi Thomas,

> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Thursday, October 06, 2016 4:53 PM
> To: Kerlin, MarcinX 
> Cc: dev at dpdk.org; De Lara Guarch, Pablo 
> Subject: Re: [PATCH v5 1/2] librte_ether: add protection against overwrite
> device data
> 
> 2016-09-30 16:00, Marcin Kerlin:
> > Added protection against overwrite device data in array
> > rte_eth_dev_data[] for the next secondary applications. Secondary
> > process appends in the first free place rather than at the beginning.
> > This behavior prevents overwriting devices data of primary process by
> secondary process.
> 
> It would be good to state what is a secondary process.
> You are trying to extend its capabilities to be able to initialize devices.
> So primary and secondary processes are almost equivalent?
> What happens if we do not create any device in the primary?
> Answer from code review: "Cannot allocate memzone for ethernet port data\n"
> 
> The secondary process is a hack to me.
> But it is fine to have such hack for debug or monitoring purpose.
> I would like to understand what are the other use cases?

It's true, it is fine for debug or monitoring but If DPDK allow run secondary 
app with 
devices then it should be safe or completely not allowed. 

This bug has been observed while running secondary testpmd with virtual devices.

I will adapt to the decision of maintainers regards to design of secondary 
process.

> 
> By the way, the code managing the shared data of a device should be at the
> EAL level in order to be used by other interfaces like crypto.
> 
> > @@ -631,6 +692,8 @@ int
> >  rte_eth_dev_detach(uint8_t port_id, char *name)  {
> > struct rte_pci_addr addr;
> > +   struct rte_eth_dev_data *eth_dev_data = NULL;
> > +   char device[RTE_ETH_NAME_MAX_LEN];
> > int ret = -1;
> >
> > if (name == NULL) {
> > @@ -642,6 +705,15 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
> > if (rte_eth_dev_is_detachable(port_id))
> > goto err;
> >
> > +   /* get device name by port id */
> > +   if (rte_eth_dev_get_name_by_port(port_id, device))
> > +   goto err;
> > +
> > +   /* look for an entry in the shared device data */
> > +   eth_dev_data = rte_eth_dev_get_dev_data_by_name(device);
> > +   if (eth_dev_data == NULL)
> > +   goto err;
> 
> Why not getting eth_dev_data from rte_eth_devices[port_id].data ?

because rte_eth_devices[port_id].data for some drivers (mainly virtual devices)
is pointer to local eth_dev_data (e.g rte_eth_pcap.c:816 and also other 
drivers).
This causes that local eth_dev_data is clearing rather than shared in memzone. 

Naming is unique so if device was added then there (shared memzone) has to be. 

> 
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> >  /**
> >   * @internal
> > + * Release device data kept in shared memory for all processes.
> > + *
> > + * @param  port_id
> > + *   The port identifier of the device to release device data.
> > + * @return
> > + *   - 0 on success, negative on error
> > + */
> > +int rte_eth_dev_release_dev_data(uint8_t port_id);
> 
> Why this function? It is not used.
> You already have done the job in the detach function.

This function is using in testpmd.c:1640, basic wrapper for clean up.

Detach function is working only for detachable devices, release_dev_data() 
no matter just clean up shared array before next run secondary e.g testpmd.

Regards,
Marcin


[dpdk-dev] [PATCH v5 1/2] librte_ether: add protection against overwrite device data

2016-10-06 Thread Kerlin, MarcinX
Hi Thomas,

> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Thursday, October 06, 2016 11:41 AM
> To: Kerlin, MarcinX 
> Cc: dev at dpdk.org; De Lara Guarch, Pablo  intel.com>;
> Gonzalez Monroy, Sergio 
> Subject: Re: [PATCH v5 1/2] librte_ether: add protection against overwrite
> device data
> 
> Hi Marcin,
> 
> 2016-09-30 16:00, Marcin Kerlin:
> > Added protection against overwrite device data in array
> > rte_eth_dev_data[] for the next secondary applications. Secondary
> > process appends in the first free place rather than at the beginning.
> > This behavior prevents overwriting devices data of primary process by
> secondary process.
> 
> I've just realized that you are trying to fix an useless code.
> Why not just remove the array rte_eth_dev_data[] at first?

because pointer to rte_eth_dev_data in rte_eth_devices[] is 
just to array rte_eth_dev_data[].

rte_ethdev.c:214 
eth_dev->data = _eth_dev_data[port_id];

> We already have the array rte_eth_devices[] and there is a pointer to
> rte_eth_dev_data in rte_eth_dev.

As you write above there is a pointer, but after run secondary testpmd this 
pointer
will indicate data which hold from now data for secondary testpmd.

1) run testpmd [primary]

rte_eth_devices[0].data.name = 3:0.1

2) run testpmd [secondary]
This testpmd overwrite first index in rte_eth_dev_data[]
3:0.1 -> eth_pcap0

3) back to testpmd [primary]
rte_eth_devices[0].data.name = eth_pcap0

from now in primary testpmd our device name is eth_pcap0 but should be 3:0.1

> 
> Is it just a workaround to be able to lookup the rte_eth_dev_data memzone in
> the secondary process?

No it is not workaround, it is protection against overwrite device data.
I think that my cover letter good explain what is wrong. I did there
short debug log. 

> So wouldn't it be saner to have rte_eth_devices[] in a memzone?

So you mean that move rte_eth_devices[] to memzone + remove rte_eth_dev_data[].
What will indicate pointer inside rte_eth_dev  rte_eth_devices[]:
(struct rte_eth_dev_data *data;  /**< Pointer to device data */)

If I did not understand you please clarify more.

Regards,
Marcin
> 
> Sergio, as the multi-process maintainer, I guess you have an idea :)



[dpdk-dev] [PATCH 0/2] add ensure consistent device data in multiprocess mode

2016-08-01 Thread Kerlin, MarcinX
Self-Nack this patch because the commit log needs change.

> -Original Message-
> From: Kerlin, MarcinX
> Sent: Friday, July 29, 2016 5:57 PM
> To: dev at dpdk.org
> Cc: De Lara Guarch, Pablo ;
> thomas.monjalon at 6wind.com; Kerlin, MarcinX 
> Subject: [PATCH 0/2] add ensure consistent device data in multiprocess mode
> 
> This patch ensure not overwrite device data in the multiprocess application.
> 
> 1)Changes in the library introduces continuity in device data
> rte_eth_dev_data[] common for to all processes. Functionality detach cleans
> data of detachable device and leaves space for other devices or for the next
> run app.
> 
> 2)Changes in application testpmd allow secondary process to attach the
> mempool created by primary process rather than create new and in the case of
> quit or force quit to free devices of this process from shared array
> rte_eth_dev_data[].
> 
> Marcin Kerlin (2):
>   lib/librte_ether: ensure not overwrite device data in multiprocess app
>   app/testpmd: fix handling of multiprocess
> 
>  app/test-pmd/testpmd.c | 30 +++-
>  app/test-pmd/testpmd.h |  1 +
>  lib/librte_ether/rte_ethdev.c  | 87 ++---
> -
>  lib/librte_ether/rte_ethdev.h  | 23 +
>  lib/librte_ether/rte_ether_version.map |  8 
>  5 files changed, 139 insertions(+), 10 deletions(-)
> 
> --
> 1.9.1



[dpdk-dev] [PATCH] ethdev: ensure consistent port id assignment

2016-07-21 Thread Kerlin, MarcinX
Hi Amin,

> -Original Message-
> From: Tootoonchian, Amin
> Sent: Wednesday, July 20, 2016 5:08 PM
> To: Kerlin, MarcinX 
> Cc: dev at dpdk.org; thomas.monjalon at 6wind.com
> Subject: RE: [PATCH] ethdev: ensure consistent port id assignment
> 
> Hi Marcin,
> 
> Comments inline:
> 
> > -Original Message-
> > From: Kerlin, MarcinX
> > Sent: Wednesday, July 20, 2016 1:51 AM
> > To: Tootoonchian, Amin 
> > Cc: dev at dpdk.org; thomas.monjalon at 6wind.com
> > Subject: RE: [PATCH] ethdev: ensure consistent port id assignment
> >
> > Hi Amin,
> >
> > > -Original Message-
> > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Tootoonchian,
> > > Amin
> > > Sent: Tuesday, July 12, 2016 4:01 AM
> > > To: thomas.monjalon at 6wind.com
> > > Cc: dev at dpdk.org
> > > Subject: [dpdk-dev] [PATCH] ethdev: ensure consistent port id
> > > assignment
> > >
> > > The rte_eth_dev_allocate() code has an implicit assumption that the
> > > port id assignment in the secondary process is consistent with that
> > > of the primary. The current code breaks if the enumeration of
> > > ethdevs in primary and secondary processes are not identical (e.g.,
> > > when the black/whitelist and vdev args of the primary and secondary
> > > do not match, or when the primary dynamically adds/removes ethdevs).
> > >
> > > To fix this problem, rte_eth_dev_allocate() now looks up port id by
> > > name in a secondary process (making it explicit that ethdevs can
> > > only be created and initialized by the primary process). Upon
> > > releasing a port, the primary process zeros out eth_dev->data to
> > > avoid false positives in port id lookup by rte_eth_dev_get_port_by_name().
> > >
> > > Signed-off-by: Amin Tootoonchian 
> > > ---
> > >  lib/librte_ether/rte_ethdev.c | 44
> > > +
> > > --
> > >  1 file changed, 30 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/lib/librte_ether/rte_ethdev.c
> > > b/lib/librte_ether/rte_ethdev.c index
> > > 0a6e3f1..1801f57 100644
> > > --- a/lib/librte_ether/rte_ethdev.c
> > > +++ b/lib/librte_ether/rte_ethdev.c
> > > @@ -195,25 +195,37 @@ rte_eth_dev_allocate(const char *name, enum
> > > rte_eth_dev_type type)
> > >   uint8_t port_id;
> > >   struct rte_eth_dev *eth_dev;
> > >
> > > - port_id = rte_eth_dev_find_free_port();
> > > - if (port_id == RTE_MAX_ETHPORTS) {
> > > - RTE_PMD_DEBUG_TRACE("Reached maximum number of
> > > Ethernet ports\n");
> > > - return NULL;
> > > - }
> > > -
> > >   if (rte_eth_dev_data == NULL)
> > >   rte_eth_dev_data_alloc();
> > >
> > > - if (rte_eth_dev_allocated(name) != NULL) {
> > > - RTE_PMD_DEBUG_TRACE("Ethernet Device with name %s
> > > already allocated!\n",
> > > - name);
> > > - return NULL;
> > > + if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> > > + port_id = rte_eth_dev_find_free_port();
> > > + if (port_id == RTE_MAX_ETHPORTS) {
> > > + RTE_PMD_DEBUG_TRACE("Reached maximum number
> > > of Ethernet ports\n");
> > > + return NULL;
> > > + }
> > > +
> > > + if (rte_eth_dev_allocated(name) != NULL) {
> > > + RTE_PMD_DEBUG_TRACE("Ethernet Device with name
> > > %s already allocated!\n",
> > > + name);
> > > + return NULL;
> > > + }
> > > + } else {
> > > + if (rte_eth_dev_get_port_by_name(name, _id) != 0) {
> >
> >
> > I was working also on this problem but I didn't send patch yet, so I
> > did review of your code.
> >
> > Condition (rte_eth_dev_get_port_by_name(name, _id) != 0) will
> > always fail.
> > Secondary process enter here and he will be looking for him name but
> > has not yet added and the application return NULL here e.g. we will
> > run app with device name pcap1 but this device is not on list and function
> return null.
> 
> This is the intended behavior with this patch. Ports are to be created only 
> by the
> primary process. This is required for correct operation IMO, because if we
> allow secondary processes to create ports dynamically (and locally

[dpdk-dev] [PATCH] ethdev: ensure consistent port id assignment

2016-07-20 Thread Kerlin, MarcinX
Hi Amin,

> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Tootoonchian, Amin
> Sent: Tuesday, July 12, 2016 4:01 AM
> To: thomas.monjalon at 6wind.com
> Cc: dev at dpdk.org
> Subject: [dpdk-dev] [PATCH] ethdev: ensure consistent port id assignment
> 
> The rte_eth_dev_allocate() code has an implicit assumption that the port id
> assignment in the secondary process is consistent with that of the primary. 
> The
> current code breaks if the enumeration of ethdevs in primary and secondary
> processes are not identical (e.g., when the black/whitelist and vdev args of 
> the
> primary and secondary do not match, or when the primary dynamically
> adds/removes ethdevs).
> 
> To fix this problem, rte_eth_dev_allocate() now looks up port id by name in a
> secondary process (making it explicit that ethdevs can only be created and
> initialized by the primary process). Upon releasing a port, the primary 
> process
> zeros out eth_dev->data to avoid false positives in port id lookup by
> rte_eth_dev_get_port_by_name().
> 
> Signed-off-by: Amin Tootoonchian 
> ---
>  lib/librte_ether/rte_ethdev.c | 44 +
> --
>  1 file changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c 
> index
> 0a6e3f1..1801f57 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -195,25 +195,37 @@ rte_eth_dev_allocate(const char *name, enum
> rte_eth_dev_type type)
>   uint8_t port_id;
>   struct rte_eth_dev *eth_dev;
> 
> - port_id = rte_eth_dev_find_free_port();
> - if (port_id == RTE_MAX_ETHPORTS) {
> - RTE_PMD_DEBUG_TRACE("Reached maximum number of
> Ethernet ports\n");
> - return NULL;
> - }
> -
>   if (rte_eth_dev_data == NULL)
>   rte_eth_dev_data_alloc();
> 
> - if (rte_eth_dev_allocated(name) != NULL) {
> - RTE_PMD_DEBUG_TRACE("Ethernet Device with name %s
> already allocated!\n",
> - name);
> - return NULL;
> + if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> + port_id = rte_eth_dev_find_free_port();
> + if (port_id == RTE_MAX_ETHPORTS) {
> + RTE_PMD_DEBUG_TRACE("Reached maximum number
> of Ethernet ports\n");
> + return NULL;
> + }
> +
> + if (rte_eth_dev_allocated(name) != NULL) {
> + RTE_PMD_DEBUG_TRACE("Ethernet Device with name
> %s already allocated!\n",
> + name);
> + return NULL;
> + }
> + } else {
> + if (rte_eth_dev_get_port_by_name(name, _id) != 0) {


I was working also on this problem but I didn't send patch yet, so I did review 
of your code.

Condition (rte_eth_dev_get_port_by_name(name, _id) != 0) will always fail.
Secondary process enter here and he will be looking for him name but has not 
yet added
and the application return NULL here e.g. we will run app with device name 
pcap1 but this 
device is not on list and function return null.


> + RTE_PMD_DEBUG_TRACE("Ethernet Device with name
> %s could not be found!\n",
> + name);
> + return NULL;
> + }
>   }
> 
>   eth_dev = _eth_devices[port_id];
>   eth_dev->data = _eth_dev_data[port_id];
> - snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s",
> name);
> - eth_dev->data->port_id = port_id;
> +
> + if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> + snprintf(eth_dev->data->name, sizeof(eth_dev->data->name),
> "%s", name);
> + eth_dev->data->port_id = port_id;
> + }
> +


1. rte_eth_dev_data[port_id] -> port id should be shifted because secondary 
process 
overwrite e.g. first position which is common with primary process, so should 
be add at the end

2. If this condition is true only for primary it means that secondary process 
can't add own name.
So this excludes with above line: "if (rte_eth_dev_get_port_by_name(name, 
_id) != 0)"?

I will send also my patch soon and we can compare and prepare a common version. 
We should 
keep in mind also the hot plugging.


>   eth_dev->attached = DEV_ATTACHED;
>   eth_dev->dev_type = type;
>   nb_ports++;
> @@ -293,8 +305,10 @@ rte_eth_dev_init(struct rte_pci_driver *pci_drv,
>   pci_drv->name,
>   (unsigned) pci_dev->id.vendor_id,
>   (unsigned) pci_dev->id.device_id);
> - if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> + if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>   rte_free(eth_dev->data->dev_private);
> + memset(eth_dev->data, 0, sizeof(*rte_eth_dev_data));
> + }
>   rte_eth_dev_release_port(eth_dev);
>   return diag;
>  }
> @@ -330,8 +344,10 @@ 

[dpdk-dev] unchecked return value in enic driver

2016-06-20 Thread Kerlin, MarcinX
Hi John and Nelson,

> -Original Message-
> From: Kerlin, MarcinX
> Sent: Monday, June 13, 2016 1:18 PM
> To: johndale at cisco.com; neescoba at cisco.com
> Cc: dev at dpdk.org
> Subject: unchecked return value in enic driver
> 
> Hi John and Nelson,
> 
> I have a question regarding Coverity defects:
> 
> File: /drivers/net/enic/enic_ethdev.c
> Line: 379
> 
> CID 13197: Unchecked return value
> (CHECKED_RETURN)1.?check_return:?Calling?rte_atomic64_cmpset?without
> checking return value (as is done elsewhere 15 out of 17 times)
> 
> Can I mark this error as "False Positive" in Coverity Classification ? reason:
> 1. Function returns a void type so change the return type to int requires
> changes all drivers 2. rte_atomic64_cmpset is at the end of function so
> nonsense added a return
> 
> What is your opinion?

I marked this Coverity as false-positive with an explanation. If in your 
opinion it is not ok, you can reopen/change/fix it.

> 
> Regards,
> Marcin


[dpdk-dev] [PATCH v2 1/1] eal: fix resource leak of mapped memory

2016-06-15 Thread Kerlin, MarcinX


> -Original Message-
> From: Panu Matilainen [mailto:pmatilai at redhat.com]
> Sent: Wednesday, June 15, 2016 12:06 PM
> To: Kerlin, MarcinX ; Gonzalez Monroy, Sergio
> ; david.marchand at 6wind.com
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/1] eal: fix resource leak of mapped
> memory
> 
> On 06/15/2016 12:35 PM, Kerlin, MarcinX wrote:
> > Hi Sergio,
> >
> > Thanks for tips, I removed double casting and I leave (void *) casting 
> > because
> pointer hp is const and compiler returns warnings.
> 
> If hp is something that needs freeing then it shouldn't be const in the first 
> place.
> Please fix that instead.

Thanks, done

> 
>   - Panu -



[dpdk-dev] [PATCH v2 1/1] eal: fix resource leak of mapped memory

2016-06-15 Thread Kerlin, MarcinX
Hi Sergio,

Thanks for tips, I removed double casting and I leave (void *) casting because 
pointer hp is const and compiler returns warnings.

Regards,
Marcin

> -Original Message-
> From: Gonzalez Monroy, Sergio
> Sent: Wednesday, June 15, 2016 10:49 AM
> To: Kerlin, MarcinX ; david.marchand at 6wind.com
> Cc: dev at dpdk.org
> Subject: Re: [PATCH v2 1/1] eal: fix resource leak of mapped memory
> 
> Hi Marcin,
> 
> On 14/06/2016 16:33, Marcin Kerlin wrote:
> > Patch fixes resource leak in rte_eal_hugepage_attach() where mapped
> > files were not freed back to the OS in case of failure. Patch uses the
> > behavior of Linux munmap: "It is not an error if the indicated range
> > does not contain any mapped pages".
> >
> > Coverity issue: 13295, 13296, 13303
> > Fixes: af75078fece3 ("first public release")
> >
> > Signed-off-by: Marcin Kerlin 
> > ---
> >   lib/librte_eal/linuxapp/eal/eal_memory.c | 11 +--
> >   1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c
> > b/lib/librte_eal/linuxapp/eal/eal_memory.c
> > index 79d1d2d..1834fa0 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal_memory.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
> > @@ -1417,7 +1417,7 @@ rte_eal_hugepage_attach(void)
> > if (internal_config.xen_dom0_support) {
> >   #ifdef RTE_LIBRTE_XEN_DOM0
> > if (rte_xen_dom0_memory_attach() < 0) {
> > -   RTE_LOG(ERR, EAL,"Failed to attach memory setments
> of primay "
> > +   RTE_LOG(ERR, EAL, "Failed to attach memory
> segments of primary "
> > "process\n");
> 
> If you want to fix spelling of error message it probably should go in a 
> different
> patch.
> 
> > return -1;
> > }
> > @@ -1481,7 +1481,7 @@ rte_eal_hugepage_attach(void)
> >
> > size = getFileSize(fd_hugepage);
> > hp = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd_hugepage, 0);
> > -   if (hp == NULL) {
> > +   if (hp == MAP_FAILED) {
> > RTE_LOG(ERR, EAL, "Could not mmap %s\n",
> eal_hugepage_info_path());
> > goto error;
> > }
> > @@ -1551,6 +1551,13 @@ rte_eal_hugepage_attach(void)
> > return 0;
> >
> >   error:
> > +   s = 0;
> > +   while (s < RTE_MAX_MEMSEG && mcfg->memseg[s].len > 0) {
> > +   munmap(mcfg->memseg[s].addr, mcfg->memseg[s].len);
> > +   s++;
> > +   }
> > +   if (hp != NULL && hp != MAP_FAILED)
> > +   munmap((void *)(uintptr_t)hp, size);
> 
> No need for double casting, nor to cast to (void *).
> 
> Sergio
> 
> > if (fd_zero >= 0)
> > close(fd_zero);
> > if (fd_hugepage >= 0)
> 



[dpdk-dev] unchecked return value in enic driver

2016-06-13 Thread Kerlin, MarcinX
Hi John and Nelson,

I have a question regarding Coverity defects:

File: /drivers/net/enic/enic_ethdev.c
Line: 379

CID 13197: Unchecked return value 
(CHECKED_RETURN)1.?check_return:?Calling?rte_atomic64_cmpset?without checking 
return value (as is done elsewhere 15 out of 17 times)

Can I mark this error as "False Positive" in Coverity Classification ? reason:
1. Function returns a void type so change the return type to int requires 
changes all drivers
2. rte_atomic64_cmpset is at the end of function so nonsense added a return

What is your opinion?

Regards,
Marcin


[dpdk-dev] [PATCH 1/1] lib/librte_eal: fix resource leak

2016-04-21 Thread Kerlin, MarcinX
> -Original Message-
> From: Gonzalez Monroy, Sergio
> Sent: Thursday, April 21, 2016 1:19 PM
> To: David Marchand 
> Cc: dev at dpdk.org; Kerlin, MarcinX 
> Subject: Re: [PATCH 1/1] lib/librte_eal: fix resource leak
> 
> On 20/04/2016 10:15, David Marchand wrote:
> > On Tue, Apr 19, 2016 at 6:27 PM, Marcin Kerlin 
> wrote:
> >> Fix issue reported by Coverity.
> >>
> >> Coverity ID 13295, 13296, 13303:
> >> Resource leak: The system resource will not be reclaimed and reused,
> >> reducing the future availability of the resource.
> >> In rte_eal_hugepage_attach: Leak of memory or pointers to system
> >> resources.
> >>
> >> Fixes: af75078fece3 ("first public release")
> >>
> >> Signed-off-by: Marcin Kerlin 
> >> ---
> >>   lib/librte_eal/linuxapp/eal/eal_memory.c | 12 +++-
> >>   1 file changed, 11 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c
> >> b/lib/librte_eal/linuxapp/eal/eal_memory.c
> >> index 5b9132c..6320aa0 100644
> >> --- a/lib/librte_eal/linuxapp/eal/eal_memory.c
> >> +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
> >> @@ -1475,13 +1475,17 @@ rte_eal_hugepage_attach(void)
> >>  "and retry running both primary "
> >>  "and secondary processes\n");
> >>  }
> >> +
> >> +   if (base_addr != MAP_FAILED)
> >> +   munmap((void *)(uintptr_t)base_addr,
> >> + mcfg->memseg[s].len);
> >> +
> > What is the point of this casting ?
> > Idem for the rest of the patch.
> 
> I don't see the point either.
> Marcin?

Oh sorry, right, an oversight with the redundant casting.

> 
> >
> > I can't see cleanup for previously mapped segments when mapping one fails.
> > Do we want this cleanup as well ?
> 
> Good point.
> 
> We are not really leaking resources because we panic if we fail to initialize 
> eal
> memory.
> 
> Now, if we are going to do the cleanup, I think that as David points out we
> should be cleaning up all previous mappings too.

Exactly app panic after fail so do we need to worry about these warnings from 
Coverity and try to improve or leave it without affecting?

> 
> Sergio
> > CC Sergio.
> >
> >



[dpdk-dev] [PATCH v2] doc/nic: add ixgbe statistics on read frequency

2016-02-29 Thread Kerlin, MarcinX
> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Harry van Haaren
> Sent: Monday, February 29, 2016 1:17 PM
> To: Mcnamara, John 
> Cc: dev at dpdk.org
> Subject: [dpdk-dev] [PATCH v2] doc/nic: add ixgbe statistics on read
> frequency
> 
> This patch adds a note to the ixgbe PMD guide, stating the minimum time
> that statistics must be polled from the hardware in order to avoid register
> values becoming saturated and "sticking" to the max value.
> 
> Reported-by: Jerry Zhang 
> Tested-by: Marcin Kerlin 
> Signed-off-by: Harry van Haaren 

Acked-by: Marcin Kerlin