[6/7] mem: add safe and unsafe versions for checking DMA mask
Checks
Commit Message
During memory initialization calling rte_mem_check_dma_mask
leads to a deadlock because memory_hotplug_lock is locked by a
writer, the current code in execution, and rte_memseg_walk
tries to lock as a reader.
This patch adds safe and unsafe versions for invoking the final
function specifying if the memory_hotplug_lock needs to be
acquired, this is for the safe version, or not, the unsafe one.
PMDs should use the safe version and just internal EAL memory
code should use the unsafe one.
Fixes: 223b7f1d5ef6 ("mem: add function for checking memseg IOVA")
Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
---
drivers/net/nfp/nfp_net.c | 2 +-
lib/librte_eal/common/eal_common_memory.c | 24 +++++++++++++++---
lib/librte_eal/common/include/rte_memory.h | 29 +++++++++++++++++++---
lib/librte_eal/common/malloc_heap.c | 2 +-
lib/librte_eal/rte_eal_version.map | 3 ++-
5 files changed, 51 insertions(+), 9 deletions(-)
Comments
On 31-Oct-18 5:29 PM, Alejandro Lucero wrote:
> During memory initialization calling rte_mem_check_dma_mask
> leads to a deadlock because memory_hotplug_lock is locked by a
> writer, the current code in execution, and rte_memseg_walk
> tries to lock as a reader.
>
> This patch adds safe and unsafe versions for invoking the final
> function specifying if the memory_hotplug_lock needs to be
> acquired, this is for the safe version, or not, the unsafe one.
> PMDs should use the safe version and just internal EAL memory
> code should use the unsafe one.
>
> Fixes: 223b7f1d5ef6 ("mem: add function for checking memseg IOVA")
>
> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> ---
I don't think _safe and _unsafe are good names. _unsafe implies
something might blow up, which isn't the case :) I think following the
naming convention established by other functions (rte_mem_check_dma_mask
and rte_mem_check_dma_mask_thread_unsafe) is better. User/driver code is
only supposed to use rte_mem_check_dma_mask safe version anyway, so
there's no need to differentiate between the two if the other one is
never supposed to be used.
> drivers/net/nfp/nfp_net.c | 2 +-
> lib/librte_eal/common/eal_common_memory.c | 24 +++++++++++++++---
> lib/librte_eal/common/include/rte_memory.h | 29 +++++++++++++++++++---
> lib/librte_eal/common/malloc_heap.c | 2 +-
> lib/librte_eal/rte_eal_version.map | 3 ++-
> 5 files changed, 51 insertions(+), 9 deletions(-)
>
<...>
> -/* check memsegs iovas are within a range based on dma mask */
> -int __rte_experimental rte_mem_check_dma_mask(uint8_t maskbits);
> +/**
> + * * @warning
Here and in other places - same issue with extra star.
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Check memsegs iovas are within a range based on dma mask.
The comments make it seem like the parameter is an actual DMA mask,
rather than DMA mask *width*. In fact, you seem to have tripped yourself
up on that already :)
Suggested rewording:
Check if all currently allocated memory segments are compliant with
supplied DMA address width.
> + *
> + * @param maskbits
> + * Address width to check against.
> + */
> +int __rte_experimental rte_mem_check_dma_mask_safe(uint8_t maskbits);
> +
> +/**
> + * * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Check memsegs iovas are within a range based on dma mask without acquiring
> + * memory_hotplug_lock first.
> + *
> + * This function is just for EAL core memory internal use. Drivers should
> + * use the previous safe one.
This is IMO too detailed. Suggested rewording:
Check if all currently allocated memory segments are compliant with
supplied DMA address width.
@warning This function is not thread-safe and is for internal use only.
> + *
> + * @param maskbits
> + * Address width to check against.
> + */
> +int __rte_experimental rte_mem_check_dma_mask_unsafe(uint8_t maskbits);
>
> /**
> * * @warning
> * @b EXPERIMENTAL: this API may change without prior notice
> *
> * Set dma mask to use once memory initialization is done.
> - * Previous function rte_mem_check_dma_mask can not be used
> + * Previous functions rte_mem_check_dma_mask_safe/unsafe can not be used
> * safely until memory has been initialized.
> */
> void __rte_experimental rte_mem_set_dma_mask(uint8_t maskbits);
> diff --git a/lib/librte_eal/common/malloc_heap.c b/lib/librte_eal/common/malloc_heap.c
> index 711622f19..dd8b983e7 100644
> --- a/lib/librte_eal/common/malloc_heap.c
> +++ b/lib/librte_eal/common/malloc_heap.c
> @@ -335,7 +335,7 @@ alloc_pages_on_heap(struct malloc_heap *heap, uint64_t pg_sz, size_t elt_size,
> * executed. For 2) implies the new memory can not be added.
> */
> if (mcfg->dma_maskbits) {
> - if (rte_mem_check_dma_mask(mcfg->dma_maskbits)) {
> + if (rte_mem_check_dma_mask_unsafe(mcfg->dma_maskbits)) {
> /* Currently this can only happen if IOMMU is enabled
> * with RTE_ARCH_X86. It is not safe to use this memory
> * so returning an error here.
> diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
> index ae24b5c73..f863903b6 100644
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -296,7 +296,8 @@ EXPERIMENTAL {
> rte_devargs_remove;
> rte_devargs_type_count;
> rte_mem_check_dma_mask;
> - rte_mem_set_dma_mask;
> + rte_mem_set_dma_mask_safe;
> + rte_mem_set_dma_mask_unsafe;
Again, alphabet :)
> rte_eal_cleanup;
> rte_fbarray_attach;
> rte_fbarray_destroy;
>
On Thu, Nov 1, 2018 at 10:38 AM Burakov, Anatoly <anatoly.burakov@intel.com>
wrote:
> On 31-Oct-18 5:29 PM, Alejandro Lucero wrote:
> > During memory initialization calling rte_mem_check_dma_mask
> > leads to a deadlock because memory_hotplug_lock is locked by a
> > writer, the current code in execution, and rte_memseg_walk
> > tries to lock as a reader.
> >
> > This patch adds safe and unsafe versions for invoking the final
> > function specifying if the memory_hotplug_lock needs to be
> > acquired, this is for the safe version, or not, the unsafe one.
> > PMDs should use the safe version and just internal EAL memory
> > code should use the unsafe one.
> >
> > Fixes: 223b7f1d5ef6 ("mem: add function for checking memseg IOVA")
> >
> > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> > ---
>
> I don't think _safe and _unsafe are good names. _unsafe implies
> something might blow up, which isn't the case :) I think following the
> naming convention established by other functions (rte_mem_check_dma_mask
> and rte_mem_check_dma_mask_thread_unsafe) is better. User/driver code is
> only supposed to use rte_mem_check_dma_mask safe version anyway, so
> there's no need to differentiate between the two if the other one is
> never supposed to be used.
>
>
It makes sense. I will do it in next version.
Thanks
> > drivers/net/nfp/nfp_net.c | 2 +-
> > lib/librte_eal/common/eal_common_memory.c | 24 +++++++++++++++---
> > lib/librte_eal/common/include/rte_memory.h | 29 +++++++++++++++++++---
> > lib/librte_eal/common/malloc_heap.c | 2 +-
> > lib/librte_eal/rte_eal_version.map | 3 ++-
> > 5 files changed, 51 insertions(+), 9 deletions(-)
> >
>
> <...>
>
> > -/* check memsegs iovas are within a range based on dma mask */
> > -int __rte_experimental rte_mem_check_dma_mask(uint8_t maskbits);
> > +/**
> > + * * @warning
>
> Here and in other places - same issue with extra star.
>
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Check memsegs iovas are within a range based on dma mask.
>
> The comments make it seem like the parameter is an actual DMA mask,
> rather than DMA mask *width*. In fact, you seem to have tripped yourself
> up on that already :)
>
> Suggested rewording:
>
> Check if all currently allocated memory segments are compliant with
> supplied DMA address width.
>
>
Ok.
> > + *
> > + * @param maskbits
> > + * Address width to check against.
> > + */
> > +int __rte_experimental rte_mem_check_dma_mask_safe(uint8_t maskbits);
> > +
> > +/**
> > + * * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Check memsegs iovas are within a range based on dma mask without
> acquiring
> > + * memory_hotplug_lock first.
> > + *
> > + * This function is just for EAL core memory internal use. Drivers
> should
> > + * use the previous safe one.
>
> This is IMO too detailed. Suggested rewording:
>
> Check if all currently allocated memory segments are compliant with
> supplied DMA address width.
>
>
Ok
> @warning This function is not thread-safe and is for internal use only.
>
> > + *
> > + * @param maskbits
> > + * Address width to check against.
> > + */
> > +int __rte_experimental rte_mem_check_dma_mask_unsafe(uint8_t maskbits);
> >
> > /**
> > * * @warning
> > * @b EXPERIMENTAL: this API may change without prior notice
> > *
> > * Set dma mask to use once memory initialization is done.
> > - * Previous function rte_mem_check_dma_mask can not be used
> > + * Previous functions rte_mem_check_dma_mask_safe/unsafe can not be
> used
> > * safely until memory has been initialized.
> > */
> > void __rte_experimental rte_mem_set_dma_mask(uint8_t maskbits);
> > diff --git a/lib/librte_eal/common/malloc_heap.c
> b/lib/librte_eal/common/malloc_heap.c
> > index 711622f19..dd8b983e7 100644
> > --- a/lib/librte_eal/common/malloc_heap.c
> > +++ b/lib/librte_eal/common/malloc_heap.c
> > @@ -335,7 +335,7 @@ alloc_pages_on_heap(struct malloc_heap *heap,
> uint64_t pg_sz, size_t elt_size,
> > * executed. For 2) implies the new memory can not be added.
> > */
> > if (mcfg->dma_maskbits) {
> > - if (rte_mem_check_dma_mask(mcfg->dma_maskbits)) {
> > + if (rte_mem_check_dma_mask_unsafe(mcfg->dma_maskbits)) {
> > /* Currently this can only happen if IOMMU is
> enabled
> > * with RTE_ARCH_X86. It is not safe to use this
> memory
> > * so returning an error here.
> > diff --git a/lib/librte_eal/rte_eal_version.map
> b/lib/librte_eal/rte_eal_version.map
> > index ae24b5c73..f863903b6 100644
> > --- a/lib/librte_eal/rte_eal_version.map
> > +++ b/lib/librte_eal/rte_eal_version.map
> > @@ -296,7 +296,8 @@ EXPERIMENTAL {
> > rte_devargs_remove;
> > rte_devargs_type_count;
> > rte_mem_check_dma_mask;
> > - rte_mem_set_dma_mask;
> > + rte_mem_set_dma_mask_safe;
> > + rte_mem_set_dma_mask_unsafe;
>
> Again, alphabet :)
>
> > rte_eal_cleanup;
> > rte_fbarray_attach;
> > rte_fbarray_destroy;
> >
>
>
> --
> Thanks,
> Anatoly
>
@@ -2703,7 +2703,7 @@ nfp_net_init(struct rte_eth_dev *eth_dev)
pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
/* NFP can not handle DMA addresses requiring more than 40 bits */
- if (rte_mem_check_dma_mask(40)) {
+ if (rte_mem_check_dma_mask_safe(40)) {
RTE_LOG(ERR, PMD, "device %s can not be used:",
pci_dev->device.name);
RTE_LOG(ERR, PMD, "\trestricted dma mask to 40 bits!\n");
@@ -446,11 +446,12 @@ check_iova(const struct rte_memseg_list *msl __rte_unused,
#endif
/* check memseg iovas are within the required range based on dma mask */
-int __rte_experimental
-rte_mem_check_dma_mask(uint8_t maskbits)
+static int __rte_experimental
+rte_mem_check_dma_mask(uint8_t maskbits, bool safe)
{
struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
uint64_t mask;
+ int ret;
/* sanity check */
if (maskbits > MAX_DMA_MASK_BITS) {
@@ -462,7 +463,12 @@ rte_mem_check_dma_mask(uint8_t maskbits)
/* create dma mask */
mask = ~((1ULL << maskbits) - 1);
- if (rte_memseg_walk(check_iova, &mask))
+ if (safe)
+ ret = rte_memseg_walk(check_iova, &mask);
+ else
+ ret = rte_memseg_walk_thread_unsafe(check_iova, &mask);
+
+ if (ret)
/*
* Dma mask precludes hugepage usage.
* This device can not be used and we do not need to keep
@@ -480,6 +486,18 @@ rte_mem_check_dma_mask(uint8_t maskbits)
return 0;
}
+int __rte_experimental
+rte_mem_check_dma_mask_safe(uint8_t maskbits)
+{
+ return rte_mem_check_dma_mask(maskbits, true);
+}
+
+int __rte_experimental
+rte_mem_check_dma_mask_unsafe(uint8_t maskbits)
+{
+ return rte_mem_check_dma_mask(maskbits, false);
+}
+
/* set dma mask to use when memory initialization is done */
void __rte_experimental
rte_mem_set_dma_mask(uint8_t maskbits)
@@ -463,15 +463,38 @@ unsigned rte_memory_get_nchannel(void);
*/
unsigned rte_memory_get_nrank(void);
-/* check memsegs iovas are within a range based on dma mask */
-int __rte_experimental rte_mem_check_dma_mask(uint8_t maskbits);
+/**
+ * * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Check memsegs iovas are within a range based on dma mask.
+ *
+ * @param maskbits
+ * Address width to check against.
+ */
+int __rte_experimental rte_mem_check_dma_mask_safe(uint8_t maskbits);
+
+/**
+ * * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Check memsegs iovas are within a range based on dma mask without acquiring
+ * memory_hotplug_lock first.
+ *
+ * This function is just for EAL core memory internal use. Drivers should
+ * use the previous safe one.
+ *
+ * @param maskbits
+ * Address width to check against.
+ */
+int __rte_experimental rte_mem_check_dma_mask_unsafe(uint8_t maskbits);
/**
* * @warning
* @b EXPERIMENTAL: this API may change without prior notice
*
* Set dma mask to use once memory initialization is done.
- * Previous function rte_mem_check_dma_mask can not be used
+ * Previous functions rte_mem_check_dma_mask_safe/unsafe can not be used
* safely until memory has been initialized.
*/
void __rte_experimental rte_mem_set_dma_mask(uint8_t maskbits);
@@ -335,7 +335,7 @@ alloc_pages_on_heap(struct malloc_heap *heap, uint64_t pg_sz, size_t elt_size,
* executed. For 2) implies the new memory can not be added.
*/
if (mcfg->dma_maskbits) {
- if (rte_mem_check_dma_mask(mcfg->dma_maskbits)) {
+ if (rte_mem_check_dma_mask_unsafe(mcfg->dma_maskbits)) {
/* Currently this can only happen if IOMMU is enabled
* with RTE_ARCH_X86. It is not safe to use this memory
* so returning an error here.
@@ -296,7 +296,8 @@ EXPERIMENTAL {
rte_devargs_remove;
rte_devargs_type_count;
rte_mem_check_dma_mask;
- rte_mem_set_dma_mask;
+ rte_mem_set_dma_mask_safe;
+ rte_mem_set_dma_mask_unsafe;
rte_eal_cleanup;
rte_fbarray_attach;
rte_fbarray_destroy;