[dpdk-dev] [PATCH 0/3] add support for devices with addressing limitations

[dpdk-dev] [PATCH 0/3] add support for devices with addressing limitations

[Alejandro Lucero]

A kernel driver uses a dma mask specifying the memory address range supported
by the device for DMA operations. With DPDK there is no possibility for doing
the same thing so it could lead to problems with those devices not being able
to use all the available physical memory.

This patchset adds support for a PMD setting a device dma mask. If this dma
mask is set this will imply a call for checking hugepages allocated are within
the supported device range.

First patch adds the checking function. If there is a hugepage (memseg) out of
the device supported range an error is raised. Nothing really we can do as any
other available hugepage (and not allocated) will be also out of range as
hugepages are ordered by physical address before allocating.

Second patch adds call to the checking function if device dma mask is set during
PMD initialization. Depending on how hugepages are created and the amount of them
the checking could slow down initialization. If a device has not addressing 
limitations the checking is not done.

Third patch adds support for setting dma mask in the PMD NFP. Current NFP card
just supports 40 bits. Future versions will support 64 bits.

Alejandro Lucero (3):
  eal/linux: add function for checking hugepages within device supported
    address range
  eth_dev: add support for device dma mask
  nfp: set device dma mask

 drivers/net/nfp/nfp_net.c                  | 11 +++++++++++
 lib/librte_eal/common/include/rte_memory.h |  6 ++++++
 lib/librte_eal/linuxapp/eal/eal_memory.c   | 27 +++++++++++++++++++++++++++
 lib/librte_ether/rte_ethdev.c              |  7 +++++++
 lib/librte_ether/rte_ethdev.h              |  1 +
 5 files changed, 52 insertions(+)

-- 
1.9.1

[dpdk-dev] [PATCH 1/3] eal/linux: add function for checking hugepages within device supported address range

[Alejandro Lucero]

 - This is needed for avoiding problems with devices not being able to address
   all the physical available memory.

Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
---
 lib/librte_eal/common/include/rte_memory.h |  6 ++++++
 lib/librte_eal/linuxapp/eal/eal_memory.c   | 27 +++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/lib/librte_eal/common/include/rte_memory.h b/lib/librte_eal/common/include/rte_memory.h
index f8dbece..67b0b28 100644
--- a/lib/librte_eal/common/include/rte_memory.h
+++ b/lib/librte_eal/common/include/rte_memory.h
@@ -256,6 +256,12 @@ rte_mem_phy2mch(uint32_t memseg_id __rte_unused, const phys_addr_t phy_addr)
 }
 #endif
 
+/**
+ * Check hugepages are within the supported
+ * device address space range.
+ */
+int rte_eal_hugepage_check_address_mask(uint64_t dma_mask);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index 5b9132c..2cd046d 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -1037,6 +1037,33 @@ calc_num_pages_per_socket(uint64_t * memory,
 }
 
 /*
+ * Some devices have addressing limitations. A PMD will indirectly call this
+ * function raising an error if any hugepage is out of address range supported.
+ * As hugepages are ordered by physical address, there is nothing to do as
+ * any other hugepage available will be out of range as well.
+ */
+int
+rte_eal_hugepage_check_address_mask(uint64_t dma_mask)
+{
+	const struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
+	phys_addr_t physaddr;
+	int i =0;
+
+	while (i < RTE_MAX_MEMSEG && mcfg->memseg[i].len > 0) {
+		physaddr = mcfg->memseg[i].phys_addr + mcfg->memseg[i].len;
+		RTE_LOG(DEBUG, EAL, "Checking page with address %"PRIx64" and device"
+				" mask 0x%"PRIx64"\n", physaddr, dma_mask);
+		if (physaddr & ~dma_mask) {
+			RTE_LOG(ERR, EAL, "Allocated hugepages are out of device address"
+					" range.");
+			return -1;
+		}
+		i++;
+	}
+	return 0;
+}
+
+/*
  * Prepare physical memory mapping: fill configuration structure with
  * these infos, return 0 on success.
  *  1. map N huge pages in separate files in hugetlbfs
-- 
1.9.1

Re: [dpdk-dev] [dpdk-dev, 1/3] eal/linux: add function for checking hugepages within device supported address range

[Jan Viktorin]

On Thu, 12 May 2016 15:33:58 +0100
"Alejandro.Lucero" <alejandro.lucero@netronome.com> wrote:

> - This is needed for avoiding problems with devices not being able to address
>    all the physical available memory.

WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#14: 
- This is needed for avoiding problems with devices not being able to address

WARNING:LONG_LINE: line over 80 characters
#57: FILE: lib/librte_eal/linuxapp/eal/eal_memory.c:1048:
+	const struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;

ERROR:SPACING: spaces required around that '=' (ctx:WxV)
#59: FILE: lib/librte_eal/linuxapp/eal/eal_memory.c:1050:
+	int i =0;
 	      ^

WARNING:LONG_LINE_STRING: line over 80 characters
#63: FILE: lib/librte_eal/linuxapp/eal/eal_memory.c:1054:
+		RTE_LOG(DEBUG, EAL, "Checking page with address %"PRIx64" and device"

WARNING:LONG_LINE_STRING: line over 80 characters
#66: FILE: lib/librte_eal/linuxapp/eal/eal_memory.c:1057:
+			RTE_LOG(ERR, EAL, "Allocated hugepages are out of device address"

total: 1 errors, 4 warnings, 45 lines checked

Regards
Jan

> 
> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> 
> ---
> lib/librte_eal/common/include/rte_memory.h |  6 ++++++
>  lib/librte_eal/linuxapp/eal/eal_memory.c   | 27 +++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/lib/librte_eal/common/include/rte_memory.h b/lib/librte_eal/common/include/rte_memory.h
> index f8dbece..67b0b28 100644
> --- a/lib/librte_eal/common/include/rte_memory.h
> +++ b/lib/librte_eal/common/include/rte_memory.h
> @@ -256,6 +256,12 @@ rte_mem_phy2mch(uint32_t memseg_id __rte_unused, const phys_addr_t phy_addr)
>  }
>  #endif
>  
> +/**
> + * Check hugepages are within the supported
> + * device address space range.
> + */
> +int rte_eal_hugepage_check_address_mask(uint64_t dma_mask);
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
> index 5b9132c..2cd046d 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_memory.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
> @@ -1037,6 +1037,33 @@ calc_num_pages_per_socket(uint64_t * memory,
>  }
>  
>  /*
> + * Some devices have addressing limitations. A PMD will indirectly call this
> + * function raising an error if any hugepage is out of address range supported.
> + * As hugepages are ordered by physical address, there is nothing to do as
> + * any other hugepage available will be out of range as well.
> + */
> +int
> +rte_eal_hugepage_check_address_mask(uint64_t dma_mask)
> +{
> +	const struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
> +	phys_addr_t physaddr;
> +	int i =0;
> +
> +	while (i < RTE_MAX_MEMSEG && mcfg->memseg[i].len > 0) {
> +		physaddr = mcfg->memseg[i].phys_addr + mcfg->memseg[i].len;
> +		RTE_LOG(DEBUG, EAL, "Checking page with address %"PRIx64" and device"
> +				" mask 0x%"PRIx64"\n", physaddr, dma_mask);
> +		if (physaddr & ~dma_mask) {
> +			RTE_LOG(ERR, EAL, "Allocated hugepages are out of device address"
> +					" range.");
> +			return -1;
> +		}
> +		i++;
> +	}
> +	return 0;
> +}
> +
> +/*
>   * Prepare physical memory mapping: fill configuration structure with
>   * these infos, return 0 on success.
>   *  1. map N huge pages in separate files in hugetlbfs



-- 
   Jan Viktorin                  E-mail: Viktorin@RehiveTech.com
   System Architect              Web:    www.RehiveTech.com
   RehiveTech
   Brno, Czech Republic

[dpdk-dev] [PATCH 2/3] eth_dev: add support for device dma mask

[Alejandro Lucero]

 - New dma_mask field in rte_eth_dev_data.
 - If PMD sets device dma_mask, call to check hugepages within
   supported range.

Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
---
 lib/librte_ether/rte_ethdev.c | 7 +++++++
 lib/librte_ether/rte_ethdev.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index a31018e..c0de88a 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -280,9 +280,16 @@ rte_eth_dev_init(struct rte_pci_driver *pci_drv,
 
 	/* Invoke PMD device initialization function */
 	diag = (*eth_drv->eth_dev_init)(eth_dev);
+	if (diag)
+		goto err;
+
+	if (eth_dev->data->dma_mask)
+		diag = rte_eal_hugepage_check_address_mask(eth_dev->data->dma_mask);
+
 	if (diag == 0)
 		return 0;
 
+err:
 	RTE_PMD_DEBUG_TRACE("driver %s: eth_dev_init(vendor_id=0x%u device_id=0x%x) failed\n",
 			pci_drv->name,
 			(unsigned) pci_dev->id.vendor_id,
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 2757510..34daa92 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1675,6 +1675,7 @@ struct rte_eth_dev_data {
 	enum rte_kernel_driver kdrv;    /**< Kernel driver passthrough */
 	int numa_node;  /**< NUMA node connection */
 	const char *drv_name;   /**< Driver name */
+	uint64_t dma_mask; /** device supported address space range */
 };
 
 /** Device supports hotplug detach */
-- 
1.9.1

Re: [dpdk-dev] [dpdk-dev, 2/3] eth_dev: add support for device dma mask

[Jan Viktorin]

Hello Alejandro,

On Thu, 12 May 2016 15:33:59 +0100
"Alejandro.Lucero" <alejandro.lucero@netronome.com> wrote:

> - New dma_mask field in rte_eth_dev_data.
>  - If PMD sets device dma_mask, call to check hugepages within

I think that one of the purposes of DPDK is to support DMA transfers
in userspace. In other words, I can see no reason to support dma_mask
at the ethdev level only.

We should consider adding this to the generic struct rte_device
(currently rte_pci_device). Thomas?

>    supported range.

I think, the '-' is unnecessary at the beginning of line. As for me
I prefer a fluent text describing the purpose. The '-' is useful for
a real list of notes.

> 
> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> 
> ---
> lib/librte_ether/rte_ethdev.c | 7 +++++++
>  lib/librte_ether/rte_ethdev.h | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index a31018e..c0de88a 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -280,9 +280,16 @@ rte_eth_dev_init(struct rte_pci_driver *pci_drv,
>  
>  	/* Invoke PMD device initialization function */
>  	diag = (*eth_drv->eth_dev_init)(eth_dev);
> +	if (diag)
> +		goto err;
> +
> +	if (eth_dev->data->dma_mask)
> +		diag = rte_eal_hugepage_check_address_mask(eth_dev->data->dma_mask);
> +

I don't understand what happens if the memory is out of the DMA mask. What can the driver
do? Does it just fail?

I think that this should be considered during a malloc instead. (Well, there is probably
no suitable API for this at the moment.)

Regards
Jan

>  	if (diag == 0)
>  		return 0;
>  
> +err:
>  	RTE_PMD_DEBUG_TRACE("driver %s: eth_dev_init(vendor_id=0x%u device_id=0x%x) failed\n",
>  			pci_drv->name,
>  			(unsigned) pci_dev->id.vendor_id,
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index 2757510..34daa92 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -1675,6 +1675,7 @@ struct rte_eth_dev_data {
>  	enum rte_kernel_driver kdrv;    /**< Kernel driver passthrough */
>  	int numa_node;  /**< NUMA node connection */
>  	const char *drv_name;   /**< Driver name */
> +	uint64_t dma_mask; /** device supported address space range */
>  };
>  
>  /** Device supports hotplug detach */



-- 
   Jan Viktorin                  E-mail: Viktorin@RehiveTech.com
   System Architect              Web:    www.RehiveTech.com
   RehiveTech
   Brno, Czech Republic

Re: [dpdk-dev] [dpdk-dev, 2/3] eth_dev: add support for device dma mask

[Alejandro Lucero]

Hi Jan

On Thu, May 12, 2016 at 3:52 PM, Jan Viktorin <viktorin@rehivetech.com>
wrote:

> Hello Alejandro,
>
> On Thu, 12 May 2016 15:33:59 +0100
> "Alejandro.Lucero" <alejandro.lucero@netronome.com> wrote:
>
> > - New dma_mask field in rte_eth_dev_data.
> >  - If PMD sets device dma_mask, call to check hugepages within
>
> I think that one of the purposes of DPDK is to support DMA transfers
> in userspace. In other words, I can see no reason to support dma_mask
> at the ethdev level only.
>
> The limitation is a device limitation so I can not see a better place for
adding the device dma mask.


> We should consider adding this to the generic struct rte_device
> (currently rte_pci_device). Thomas?
>
> I guess it could be a non-pci device with such a limitation. I though
rte_ethdev is more generic.


> >    supported range.
>
> I think, the '-' is unnecessary at the beginning of line. As for me
> I prefer a fluent text describing the purpose. The '-' is useful for
> a real list of notes.
>
> >
> > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> >
> > ---
> > lib/librte_ether/rte_ethdev.c | 7 +++++++
> >  lib/librte_ether/rte_ethdev.h | 1 +
> >  2 files changed, 8 insertions(+)
> >
> > diff --git a/lib/librte_ether/rte_ethdev.c
> b/lib/librte_ether/rte_ethdev.c
> > index a31018e..c0de88a 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -280,9 +280,16 @@ rte_eth_dev_init(struct rte_pci_driver *pci_drv,
> >
> >       /* Invoke PMD device initialization function */
> >       diag = (*eth_drv->eth_dev_init)(eth_dev);
> > +     if (diag)
> > +             goto err;
> > +
> > +     if (eth_dev->data->dma_mask)
> > +             diag =
> rte_eal_hugepage_check_address_mask(eth_dev->data->dma_mask);
> > +
>
> I don't understand what happens if the memory is out of the DMA mask. What
> can the driver
> do? Does it just fail?
>
> I think that this should be considered during a malloc instead. (Well,
> there is probably
> no suitable API for this at the moment.)
>
> hugepage memory allocation is done before device initialization. I see
easier to leave the normal hugepage code as it is now and add a later call
if a device requires it.

The only reasonable thing to do is to fail as the amount of required memory
can not be (safely) allocated.


> Regards
> Jan
>
> >       if (diag == 0)
> >               return 0;
> >
> > +err:
> >       RTE_PMD_DEBUG_TRACE("driver %s: eth_dev_init(vendor_id=0x%u
> device_id=0x%x) failed\n",
> >                       pci_drv->name,
> >                       (unsigned) pci_dev->id.vendor_id,
> > diff --git a/lib/librte_ether/rte_ethdev.h
> b/lib/librte_ether/rte_ethdev.h
> > index 2757510..34daa92 100644
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > @@ -1675,6 +1675,7 @@ struct rte_eth_dev_data {
> >       enum rte_kernel_driver kdrv;    /**< Kernel driver passthrough */
> >       int numa_node;  /**< NUMA node connection */
> >       const char *drv_name;   /**< Driver name */
> > +     uint64_t dma_mask; /** device supported address space range */
> >  };
> >
> >  /** Device supports hotplug detach */
>
>
>
> --
>    Jan Viktorin                  E-mail: Viktorin@RehiveTech.com
>    System Architect              Web:    www.RehiveTech.com
>    RehiveTech
>    Brno, Czech Republic
>

Re: [dpdk-dev] [dpdk-dev, 2/3] eth_dev: add support for device dma mask

[Jan Viktorin]

Hi,

Just a note, please, when replying inline, do not prepend ">" before your
new text. I could not find your replies.

See below...

On Thu, 12 May 2016 16:03:14 +0100
Alejandro Lucero <alejandro.lucero@netronome.com> wrote:

> Hi Jan
> 
> On Thu, May 12, 2016 at 3:52 PM, Jan Viktorin <viktorin@rehivetech.com>
> wrote:
> 
> > Hello Alejandro,
> >
> > On Thu, 12 May 2016 15:33:59 +0100
> > "Alejandro.Lucero" <alejandro.lucero@netronome.com> wrote:
> >  
> > > - New dma_mask field in rte_eth_dev_data.
> > >  - If PMD sets device dma_mask, call to check hugepages within  
> >
> > I think that one of the purposes of DPDK is to support DMA transfers
> > in userspace. In other words, I can see no reason to support dma_mask
> > at the ethdev level only.
> >
> > The limitation is a device limitation so I can not see a better place for  
> adding the device dma mask.

That's what I've meant. It is a _device_ limitation. The ethdev is a wrapper
around the rte_pci_device. The ethdev just extends semantics of the generic device.
However, all DPDK devices are expected to be DMA-capable.

If you get a pointer to the ethdev, you get a pointer to the rte_pci_device as well
(1 more level of dereference but we are not on the fast path here, so it's unimportant).

Consider the cryptodev. If cryptodev has some DMA mask requirements we can support it
in the generic place, i.e. rte_pci_device and not rte_ethdev because the cryptodev
is not an ethdev.

> 
> 
> > We should consider adding this to the generic struct rte_device
> > (currently rte_pci_device). Thomas?
> >
> > I guess it could be a non-pci device with such a limitation. I though  
> rte_ethdev is more generic.

When it is added to the rte_pci_device (or rte_device after the planned changes)
the non-PCI devices get this for free... Otherwise I don't understand the point
here.

> 
> 
> > >    supported range.  
> >
> > I think, the '-' is unnecessary at the beginning of line. As for me
> > I prefer a fluent text describing the purpose. The '-' is useful for
> > a real list of notes.
> >  
> > >
> > > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> > >
> > > ---
> > > lib/librte_ether/rte_ethdev.c | 7 +++++++
> > >  lib/librte_ether/rte_ethdev.h | 1 +
> > >  2 files changed, 8 insertions(+)
> > >
> > > diff --git a/lib/librte_ether/rte_ethdev.c  
> > b/lib/librte_ether/rte_ethdev.c  
> > > index a31018e..c0de88a 100644
> > > --- a/lib/librte_ether/rte_ethdev.c
> > > +++ b/lib/librte_ether/rte_ethdev.c
> > > @@ -280,9 +280,16 @@ rte_eth_dev_init(struct rte_pci_driver *pci_drv,
> > >
> > >       /* Invoke PMD device initialization function */
> > >       diag = (*eth_drv->eth_dev_init)(eth_dev);
> > > +     if (diag)
> > > +             goto err;
> > > +
> > > +     if (eth_dev->data->dma_mask)
> > > +             diag =  
> > rte_eal_hugepage_check_address_mask(eth_dev->data->dma_mask);  
> > > +  
> >
> > I don't understand what happens if the memory is out of the DMA mask. What
> > can the driver
> > do? Does it just fail?
> >
> > I think that this should be considered during a malloc instead. (Well,
> > there is probably
> > no suitable API for this at the moment.)
> >
> > hugepage memory allocation is done before device initialization. I see  
> easier to leave the normal hugepage code as it is now and add a later call
> if a device requires it.

True. I didn't meant to change hugepages allocation but to change allocation
of memory for a device. If we reserve a set of hugepages, a subset of them can
match the DMA mask for a particular device. When allocating memory for a device,
we can consider the DMA mask and choose only the hugepages we can handle.

Quite frankly, I am not very aware of the memory allocation internals of DPDK so
I can be wrong...

> 
> The only reasonable thing to do is to fail as the amount of required memory
> can not be (safely) allocated.

This is the easiest way and it is not bad. However, what would be a workaround?
If a user gets some allocated memory but out of the DMA mask, how can she fix it?
I don't know... Is it deterministic? Would it always allocate memory out of the
mask or only sometimes?

Jan

> 
> 
> > Regards
> > Jan
> >  
> > >       if (diag == 0)
> > >               return 0;
> > >
> > > +err:
> > >       RTE_PMD_DEBUG_TRACE("driver %s: eth_dev_init(vendor_id=0x%u  
> > device_id=0x%x) failed\n",  
> > >                       pci_drv->name,
> > >                       (unsigned) pci_dev->id.vendor_id,
> > > diff --git a/lib/librte_ether/rte_ethdev.h  
> > b/lib/librte_ether/rte_ethdev.h  
> > > index 2757510..34daa92 100644
> > > --- a/lib/librte_ether/rte_ethdev.h
> > > +++ b/lib/librte_ether/rte_ethdev.h
> > > @@ -1675,6 +1675,7 @@ struct rte_eth_dev_data {
> > >       enum rte_kernel_driver kdrv;    /**< Kernel driver passthrough */
> > >       int numa_node;  /**< NUMA node connection */
> > >       const char *drv_name;   /**< Driver name */
> > > +     uint64_t dma_mask; /** device supported address space range */
> > >  };
> > >
> > >  /** Device supports hotplug detach */  
> >
> >
> >
> > --
> >    Jan Viktorin                  E-mail: Viktorin@RehiveTech.com
> >    System Architect              Web:    www.RehiveTech.com
> >    RehiveTech
> >    Brno, Czech Republic
> >  



-- 
   Jan Viktorin                  E-mail: Viktorin@RehiveTech.com
   System Architect              Web:    www.RehiveTech.com
   RehiveTech
   Brno, Czech Republic

Re: [dpdk-dev] [dpdk-dev, 2/3] eth_dev: add support for device dma mask

[Alejandro Lucero]

On Thu, May 12, 2016 at 4:41 PM, Jan Viktorin <viktorin@rehivetech.com>
wrote:

> Hi,
>
> Just a note, please, when replying inline, do not prepend ">" before your
> new text. I could not find your replies.
>
>
My gmail interface does not show that prepend character... It seems I have
to leave a white line before my replies.



> See below...
>
> On Thu, 12 May 2016 16:03:14 +0100
> Alejandro Lucero <alejandro.lucero@netronome.com> wrote:
>
> > Hi Jan
> >
> > On Thu, May 12, 2016 at 3:52 PM, Jan Viktorin <viktorin@rehivetech.com>
> > wrote:
> >
> > > Hello Alejandro,
> > >
> > > On Thu, 12 May 2016 15:33:59 +0100
> > > "Alejandro.Lucero" <alejandro.lucero@netronome.com> wrote:
> > >
> > > > - New dma_mask field in rte_eth_dev_data.
> > > >  - If PMD sets device dma_mask, call to check hugepages within
> > >
> > > I think that one of the purposes of DPDK is to support DMA transfers
> > > in userspace. In other words, I can see no reason to support dma_mask
> > > at the ethdev level only.
> > >
> > > The limitation is a device limitation so I can not see a better place
> for
> > adding the device dma mask.
>
> That's what I've meant. It is a _device_ limitation. The ethdev is a
> wrapper
> around the rte_pci_device. The ethdev just extends semantics of the
> generic device.
> However, all DPDK devices are expected to be DMA-capable.
>
> If you get a pointer to the ethdev, you get a pointer to the
> rte_pci_device as well
> (1 more level of dereference but we are not on the fast path here, so it's
> unimportant).
>
> Consider the cryptodev. If cryptodev has some DMA mask requirements we can
> support it
> in the generic place, i.e. rte_pci_device and not rte_ethdev because the
> cryptodev
> is not an ethdev.
>
>
Ok. I was wrongly assuming we had just ethdevs, with the ethdev being the
generic and rte_pci_device being a type of ethdev.

I can add the dma mask to the rte_pci_dev. The extra level of dereference
is not a problem as long as we do not use that dma mask for a more complex
allocation API (more about this later).

If I understand it right, work is in progress for adding a rte_device. I
can not see a problem with adding dma mask to the rte_device struct either.



> >
> >
> > > We should consider adding this to the generic struct rte_device
> > > (currently rte_pci_device). Thomas?
> > >
> > > I guess it could be a non-pci device with such a limitation. I though
> > rte_ethdev is more generic.
>
> When it is added to the rte_pci_device (or rte_device after the planned
> changes)
> the non-PCI devices get this for free... Otherwise I don't understand the
> point
> here.
>
> >
> >
> > > >    supported range.
> > >
> > > I think, the '-' is unnecessary at the beginning of line. As for me
> > > I prefer a fluent text describing the purpose. The '-' is useful for
> > > a real list of notes.
> > >
> > > >
> > > > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> > > >
> > > > ---
> > > > lib/librte_ether/rte_ethdev.c | 7 +++++++
> > > >  lib/librte_ether/rte_ethdev.h | 1 +
> > > >  2 files changed, 8 insertions(+)
> > > >
> > > > diff --git a/lib/librte_ether/rte_ethdev.c
> > > b/lib/librte_ether/rte_ethdev.c
> > > > index a31018e..c0de88a 100644
> > > > --- a/lib/librte_ether/rte_ethdev.c
> > > > +++ b/lib/librte_ether/rte_ethdev.c
> > > > @@ -280,9 +280,16 @@ rte_eth_dev_init(struct rte_pci_driver *pci_drv,
> > > >
> > > >       /* Invoke PMD device initialization function */
> > > >       diag = (*eth_drv->eth_dev_init)(eth_dev);
> > > > +     if (diag)
> > > > +             goto err;
> > > > +
> > > > +     if (eth_dev->data->dma_mask)
> > > > +             diag =
> > > rte_eal_hugepage_check_address_mask(eth_dev->data->dma_mask);
> > > > +
> > >
> > > I don't understand what happens if the memory is out of the DMA mask.
> What
> > > can the driver
> > > do? Does it just fail?
> > >
> > > I think that this should be considered during a malloc instead. (Well,
> > > there is probably
> > > no suitable API for this at the moment.)
> > >
> > > hugepage memory allocation is done before device initialization. I see
> > easier to leave the normal hugepage code as it is now and add a later
> call
> > if a device requires it.
>
> True. I didn't meant to change hugepages allocation but to change
> allocation
> of memory for a device. If we reserve a set of hugepages, a subset of them
> can
> match the DMA mask for a particular device. When allocating memory for a
> device,
> we can consider the DMA mask and choose only the hugepages we can handle.
>
> Quite frankly, I am not very aware of the memory allocation internals of
> DPDK so
> I can be wrong...
>
>
Of course, we could use a device dma mask smartly, and for ethdevs, to
allocate device TX and RX rings and mbufs just from mempools with hugepages
in the supported range. Any memory allocation by the app not related to
packet handling could use another mempool with no dma mask restriction at
all. This implies awareness by the app in some way, what I guess it would
preferable to avoid.

In our case, NFP PMD support, the restriction is 40bits which implies
potential problems with machines above 1TB of memory. These type of
machines are not likely to be used with our card, and even with those
machines a DPDK app could work smoothly while allocated hugepages are below
the 1TB. Even more, DPDK in VMs inside those machines will not have
problems because the IOMMU (assuming SRIOV) will map the guest physical
address which will be (not 100% sure about this for any hypervisor) below
1TB (assuming VMs do not get more than 1TB each, of course). So, a solution
where this is managed "roughly" with an error if an allocated hugepage is
out of range is acceptable for us. We first thought to just check the
amount of memory in the system inside our PMD and return an error if more
than 1TB but I prefer to raise an error just if we really need to regarding
the allocated hugepages.

I understand this could not be the case for other devices with more
likeliness to have problems, and then a memory allocation API using devices
dma mask makes sense. We could consider this a second step to support
device limitation addresses and to implement that if really needed.



> >
> > The only reasonable thing to do is to fail as the amount of required
> memory
> > can not be (safely) allocated.
>
> This is the easiest way and it is not bad. However, what would be a
> workaround?
> If a user gets some allocated memory but out of the DMA mask, how can she
> fix it?
> I don't know... Is it deterministic? Would it always allocate memory out
> of the
> mask or only sometimes?
>
> Jan
>
> >
> >
> > > Regards
> > > Jan
> > >
> > > >       if (diag == 0)
> > > >               return 0;
> > > >
> > > > +err:
> > > >       RTE_PMD_DEBUG_TRACE("driver %s: eth_dev_init(vendor_id=0x%u
> > > device_id=0x%x) failed\n",
> > > >                       pci_drv->name,
> > > >                       (unsigned) pci_dev->id.vendor_id,
> > > > diff --git a/lib/librte_ether/rte_ethdev.h
> > > b/lib/librte_ether/rte_ethdev.h
> > > > index 2757510..34daa92 100644
> > > > --- a/lib/librte_ether/rte_ethdev.h
> > > > +++ b/lib/librte_ether/rte_ethdev.h
> > > > @@ -1675,6 +1675,7 @@ struct rte_eth_dev_data {
> > > >       enum rte_kernel_driver kdrv;    /**< Kernel driver passthrough
> */
> > > >       int numa_node;  /**< NUMA node connection */
> > > >       const char *drv_name;   /**< Driver name */
> > > > +     uint64_t dma_mask; /** device supported address space range */
> > > >  };
> > > >
> > > >  /** Device supports hotplug detach */
> > >
> > >
> > >
> > > --
> > >    Jan Viktorin                  E-mail: Viktorin@RehiveTech.com
> > >    System Architect              Web:    www.RehiveTech.com
> > >    RehiveTech
> > >    Brno, Czech Republic
> > >
>
>
>
> --
>    Jan Viktorin                  E-mail: Viktorin@RehiveTech.com
>    System Architect              Web:    www.RehiveTech.com
>    RehiveTech
>    Brno, Czech Republic
>

Re: [dpdk-dev] [dpdk-dev, 2/3] eth_dev: add support for device dma mask

[Thomas Monjalon]

2016-05-13 09:38, Alejandro Lucero:
> On Thu, May 12, 2016 at 4:41 PM, Jan Viktorin <viktorin@rehivetech.com>
> > Alejandro Lucero <alejandro.lucero@netronome.com> wrote:
> > > On Thu, May 12, 2016 at 3:52 PM, Jan Viktorin <viktorin@rehivetech.com>
> > > > "Alejandro.Lucero" <alejandro.lucero@netronome.com> wrote:
> > > > > - New dma_mask field in rte_eth_dev_data.
> > > > >  - If PMD sets device dma_mask, call to check hugepages within
> > > >
> > > > I think that one of the purposes of DPDK is to support DMA transfers
> > > > in userspace. In other words, I can see no reason to support dma_mask
> > > > at the ethdev level only.
> > > >
> > > > The limitation is a device limitation so I can not see a better place
> > for
> > > adding the device dma mask.
> >
> > That's what I've meant. It is a _device_ limitation. The ethdev is a
> > wrapper
> > around the rte_pci_device. The ethdev just extends semantics of the
> > generic device.
> > However, all DPDK devices are expected to be DMA-capable.
> >
> > If you get a pointer to the ethdev, you get a pointer to the
> > rte_pci_device as well
> > (1 more level of dereference but we are not on the fast path here, so it's
> > unimportant).
> >
> > Consider the cryptodev. If cryptodev has some DMA mask requirements we can
> > support it
> > in the generic place, i.e. rte_pci_device and not rte_ethdev because the
> > cryptodev
> > is not an ethdev.
> >
> Ok. I was wrongly assuming we had just ethdevs, with the ethdev being the
> generic and rte_pci_device being a type of ethdev.
> 
> I can add the dma mask to the rte_pci_dev. The extra level of dereference
> is not a problem as long as we do not use that dma mask for a more complex
> allocation API (more about this later).
> 
> If I understand it right, work is in progress for adding a rte_device. I
> can not see a problem with adding dma mask to the rte_device struct either.
> 
> > > > We should consider adding this to the generic struct rte_device
> > > > (currently rte_pci_device). Thomas?

Yes
This patchset could be split in 2 discussions:
- ability to restrict the physical address range of requested memory,
see the memory allocation rework discussion:
	http://dpdk.org/ml/archives/dev/2016-April/037444.html
- DMA range capability in a device, to be done on top of the EAL/device
rework in progress.

This feature is a good candidate for the roadmap of 16.11.

[dpdk-dev] [PATCH 3/3] nfp: set device dma mask

[Alejandro Lucero]

 - Just hugepages within the supported range will be available.

Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
---
 drivers/net/nfp/nfp_net.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index ea5a2a3..e0e444a 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -115,6 +115,14 @@ enum nfp_qcp_ptr {
 	NFP_QCP_WRITE_PTR
 };
 
+#ifndef DMA_64BIT_MASK
+#define DMA_64BIT_MASK  0xffffffffffffffffULL
+#endif
+
+#ifndef DMA_BIT_MASK
+#define DMA_BIT_MASK(n) (((n) == 64) ? DMA_64BIT_MASK : ((1ULL<<(n))-1))
+#endif
+
 /*
  * nfp_qcp_ptr_add - Add the value to the selected pointer of a queue
  * @q: Base address for queue structure
@@ -2441,6 +2449,9 @@ nfp_net_init(struct rte_eth_dev *eth_dev)
 	/* Recording current stats counters values */
 	nfp_net_stats_reset(eth_dev);
 
+	/* Setting dma_mask */
+	eth_dev->data->dma_mask = DMA_BIT_MASK(40);
+
 	return 0;
 }
 
-- 
1.9.1

Re: [dpdk-dev] [dpdk-dev,3/3] nfp: set device dma mask

[Jan Viktorin]

On Thu, 12 May 2016 15:34:00 +0100
"Alejandro.Lucero" <alejandro.lucero@netronome.com> wrote:

> - Just hugepages within the supported range will be available.

Again the hyphen is redundant here.

By the way, this text does not describe the change well. If I understood
the whole patch set (I am not quite sure now), the initialization would
fail if there are hugepages out of the given DMA mask. Am I wrong?

I'd expect something like "NFP supports DMA address in range ...".

> 
> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> 
> ---
> drivers/net/nfp/nfp_net.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
> index ea5a2a3..e0e444a 100644
> --- a/drivers/net/nfp/nfp_net.c
> +++ b/drivers/net/nfp/nfp_net.c
> @@ -115,6 +115,14 @@ enum nfp_qcp_ptr {
>  	NFP_QCP_WRITE_PTR
>  };
>  
> +#ifndef DMA_64BIT_MASK
> +#define DMA_64BIT_MASK  0xffffffffffffffffULL
> +#endif
> +
> +#ifndef DMA_BIT_MASK
> +#define DMA_BIT_MASK(n) (((n) == 64) ? DMA_64BIT_MASK : ((1ULL<<(n))-1))
> +#endif

This is quite a generic code, I'd put it into the EAL. Probably, it should
be renamed to something like RTE_DMA_BIT_MASK.

> +
>  /*
>   * nfp_qcp_ptr_add - Add the value to the selected pointer of a queue
>   * @q: Base address for queue structure
> @@ -2441,6 +2449,9 @@ nfp_net_init(struct rte_eth_dev *eth_dev)
>  	/* Recording current stats counters values */
>  	nfp_net_stats_reset(eth_dev);
>  
> +	/* Setting dma_mask */
> +	eth_dev->data->dma_mask = DMA_BIT_MASK(40);

Can we read this from /sys/bus/pci/devices/*/dma_mask_bits? I am not sure
whether is this generic enough but I can see dma_mask_bits for the PCI
devices on my PC.

Regards
Jan

> +
>  	return 0;
>  }
>  




-- 
   Jan Viktorin                  E-mail: Viktorin@RehiveTech.com
   System Architect              Web:    www.RehiveTech.com
   RehiveTech
   Brno, Czech Republic

Re: [dpdk-dev] [dpdk-dev,3/3] nfp: set device dma mask

[Alejandro Lucero]

On Thu, May 12, 2016 at 4:03 PM, Jan Viktorin <viktorin@rehivetech.com>
wrote:

> On Thu, 12 May 2016 15:34:00 +0100
> "Alejandro.Lucero" <alejandro.lucero@netronome.com> wrote:
>
> > - Just hugepages within the supported range will be available.
>
> Again the hyphen is redundant here.
>
> By the way, this text does not describe the change well. If I understood
> the whole patch set (I am not quite sure now), the initialization would
> fail if there are hugepages out of the given DMA mask. Am I wrong?
>
>
You are right.


> I'd expect something like "NFP supports DMA address in range ...".
>
>
That is a good idea. I was thinking on adding a memseg dump info as well
which would help to understand this issue and other related to memory
allocation.


> >
> > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> >
> > ---
> > drivers/net/nfp/nfp_net.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
> > index ea5a2a3..e0e444a 100644
> > --- a/drivers/net/nfp/nfp_net.c
> > +++ b/drivers/net/nfp/nfp_net.c
> > @@ -115,6 +115,14 @@ enum nfp_qcp_ptr {
> >       NFP_QCP_WRITE_PTR
> >  };
> >
> > +#ifndef DMA_64BIT_MASK
> > +#define DMA_64BIT_MASK  0xffffffffffffffffULL
> > +#endif
> > +
> > +#ifndef DMA_BIT_MASK
> > +#define DMA_BIT_MASK(n) (((n) == 64) ? DMA_64BIT_MASK : ((1ULL<<(n))-1))
> > +#endif
>
> This is quite a generic code, I'd put it into the EAL. Probably, it should
> be renamed to something like RTE_DMA_BIT_MASK.


OK.


>
>
> +
> >  /*
> >   * nfp_qcp_ptr_add - Add the value to the selected pointer of a queue
> >   * @q: Base address for queue structure
> > @@ -2441,6 +2449,9 @@ nfp_net_init(struct rte_eth_dev *eth_dev)
> >       /* Recording current stats counters values */
> >       nfp_net_stats_reset(eth_dev);
> >
> > +     /* Setting dma_mask */
> > +     eth_dev->data->dma_mask = DMA_BIT_MASK(40);
>
> Can we read this from /sys/bus/pci/devices/*/dma_mask_bits? I am not sure
> whether is this generic enough but I can see dma_mask_bits for the PCI
> devices on my PC.
>
>
The kernel adds a default dma mask when device scanning (at least for PCI
devices). It is a device driver who knows about specific DMA addressing
limitations. For example, this is done with UIO (igb_uio) and the using
sysfs would be fine (but then you should add support for specifying a dma
mask in igb_uio as a module param) but this is not true for VFIO.



> Regards
> Jan
>
> > +
> >       return 0;
> >  }
> >
>
>
>
>
> --
>    Jan Viktorin                  E-mail: Viktorin@RehiveTech.com
>    System Architect              Web:    www.RehiveTech.com
>    RehiveTech
>    Brno, Czech Republic
>

Re: [dpdk-dev] [dpdk-dev,3/3] nfp: set device dma mask

[Jan Viktorin]

On Thu, 12 May 2016 16:13:55 +0100
Alejandro Lucero <alejandro.lucero@netronome.com> wrote:

> On Thu, May 12, 2016 at 4:03 PM, Jan Viktorin <viktorin@rehivetech.com>
> wrote:
> 
> > On Thu, 12 May 2016 15:34:00 +0100
> > "Alejandro.Lucero" <alejandro.lucero@netronome.com> wrote:
> >  
> > > - Just hugepages within the supported range will be available.  
> >
> > Again the hyphen is redundant here.
> >
> > By the way, this text does not describe the change well. If I understood
> > the whole patch set (I am not quite sure now), the initialization would
> > fail if there are hugepages out of the given DMA mask. Am I wrong?
> >
> >  
> You are right.
> 
> 
> > I'd expect something like "NFP supports DMA address in range ...".
> >
> >  
> That is a good idea. I was thinking on adding a memseg dump info as well
> which would help to understand this issue and other related to memory
> allocation.
> 
> 
> > >
> > > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> > >
> > > ---
> > > drivers/net/nfp/nfp_net.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
> > > index ea5a2a3..e0e444a 100644
> > > --- a/drivers/net/nfp/nfp_net.c
> > > +++ b/drivers/net/nfp/nfp_net.c
> > > @@ -115,6 +115,14 @@ enum nfp_qcp_ptr {
> > >       NFP_QCP_WRITE_PTR
> > >  };
> > >
> > > +#ifndef DMA_64BIT_MASK
> > > +#define DMA_64BIT_MASK  0xffffffffffffffffULL
> > > +#endif
> > > +
> > > +#ifndef DMA_BIT_MASK
> > > +#define DMA_BIT_MASK(n) (((n) == 64) ? DMA_64BIT_MASK : ((1ULL<<(n))-1))
> > > +#endif  
> >
> > This is quite a generic code, I'd put it into the EAL. Probably, it should
> > be renamed to something like RTE_DMA_BIT_MASK.  
> 
> 
> OK.

By the way:

CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#33: FILE: drivers/net/nfp/nfp_net.c:123:
+#define DMA_BIT_MASK(n) (((n) == 64) ? DMA_64BIT_MASK : ((1ULL<<(n))-1))
                                                               ^

CHECK:SPACING: spaces preferred around that '-' (ctx:VxV)
#33: FILE: drivers/net/nfp/nfp_net.c:123:
+#define DMA_BIT_MASK(n) (((n) == 64) ? DMA_64BIT_MASK : ((1ULL<<(n))-1))
                                                                     ^

total: 0 errors, 0 warnings, 2 checks, 23 lines checked

> 
> 
> >
> >
> > +  
> > >  /*
> > >   * nfp_qcp_ptr_add - Add the value to the selected pointer of a queue
> > >   * @q: Base address for queue structure
> > > @@ -2441,6 +2449,9 @@ nfp_net_init(struct rte_eth_dev *eth_dev)
> > >       /* Recording current stats counters values */
> > >       nfp_net_stats_reset(eth_dev);
> > >
> > > +     /* Setting dma_mask */
> > > +     eth_dev->data->dma_mask = DMA_BIT_MASK(40);  
> >
> > Can we read this from /sys/bus/pci/devices/*/dma_mask_bits? I am not sure
> > whether is this generic enough but I can see dma_mask_bits for the PCI
> > devices on my PC.
> >
> >  
> The kernel adds a default dma mask when device scanning (at least for PCI
> devices). It is a device driver who knows about specific DMA addressing
> limitations. For example, this is done with UIO (igb_uio) and the using
> sysfs would be fine (but then you should add support for specifying a dma
> mask in igb_uio as a module param) but this is not true for VFIO.
> 

Makes sense...

Jan

> 
> 
> > Regards
> > Jan
> >  
> > > +
> > >       return 0;
> > >  }
> > >  
> >
> >
> >
> >
> > --
> >    Jan Viktorin                  E-mail: Viktorin@RehiveTech.com
> >    System Architect              Web:    www.RehiveTech.com
> >    RehiveTech
> >    Brno, Czech Republic
> >  



-- 
   Jan Viktorin                  E-mail: Viktorin@RehiveTech.com
   System Architect              Web:    www.RehiveTech.com
   RehiveTech
   Brno, Czech Republic