RE: [PATCH v20] eal: RTE_PTR_ADD/SUB API improvements

["Morten Brørup" <mb@smartsharesystems.com>]

> +* eal: Improved pointer arithmetic macros to preserve pointer
> provenance and type qualifiers.
> +
> +  * ``RTE_PTR_ADD``, ``RTE_PTR_SUB``, ``RTE_PTR_ALIGN``,
> ``RTE_PTR_ALIGN_CEIL``,
> +    and ``RTE_PTR_ALIGN_FLOOR`` now preserve const/volatile qualifiers
> and use
> +    pointer arithmetic instead of integer casts to enable compiler
> optimizations. These
> +    macros do not nest infinitely and may require intermediate
> variables.
> +  * Passing NULL to ``RTE_PTR_ADD``, ``RTE_PTR_SUB``,
> ``RTE_PTR_ALIGN``,
> +    ``RTE_PTR_ALIGN_CEIL``, or ``RTE_PTR_ALIGN_FLOOR`` clarified as
> undefined behavior.
> +  * Existing code passing integer types as pointer to ``RTE_PTR_ADD``
> or ``RTE_PTR_SUB``
> +    should use native operators (e.g. + -). Use of ``RTE_PTR_ALIGN``,
> ``RTE_PTR_ALIGN_CEIL``
> +    or ``RTE_PTR_ALIGN_FLOOR`` should use ``RTE_ALIGN_CEIL`` or
> ``RTE_ALIGN_FLOOR``.

Clarify (for dummies):
"Use of RTE_PTR_ALIGN [...]
+ with integer types
should use [...]"


> --- a/drivers/common/cnxk/roc_platform.h
> +++ b/drivers/common/cnxk/roc_platform.h
> @@ -47,6 +47,8 @@
>  #define PLT_PTR_ADD		 RTE_PTR_ADD
>  #define PLT_PTR_SUB		 RTE_PTR_SUB
>  #define PLT_PTR_DIFF		 RTE_PTR_DIFF
> +#define PLT_PTR_UNQUAL		 RTE_PTR_UNQUAL

The PLT_PTR_UNQUAL macro is only used in drivers/common/cnxk/roc_cpt_debug.c; but I think it can be avoided by changing the frag_info variable from "struct cpt_frag_info_s *frag_info;" to "const struct cpt_frag_info_s *frag_info;". Then you don't need to add the PLT_PTR_UNQUAL macro.

> +#define PLT_INT_PTR(intptr)	 ((void *)(uintptr_t)(intptr))

Like I didn't like the RTE_INT_PTR macros, I also don't like the PLT_INT_PTR() macro. Please get rid of it.
If an address variable is uintptr_t type, can't you cast it to void* and still use PLT_PTR_ADD()?


> --- a/drivers/net/ena/ena_ethdev.c
> +++ b/drivers/net/ena/ena_ethdev.c
> @@ -2379,7 +2379,14 @@ static void *pci_bar_addr(struct rte_pci_device
> *dev, uint32_t bar)
>  {
>  	const struct rte_mem_resource *res = &dev->mem_resource[bar];
>  	size_t offset = res->phys_addr % rte_mem_page_size();
> -	void *vaddr = RTE_PTR_ADD(res->addr, offset);
> +	void *vaddr;
> +
> +	if (res->addr == NULL) {
> +		PMD_INIT_LOG_LINE(ERR, "PCI BAR [%u] address is NULL",
> bar);
> +		return NULL;
> +	}
> +
> +	vaddr = RTE_PTR_ADD(res->addr, offset);

Note to other reviewers:
The added check for res->addr == NULL looks like an unrelated change.
But it also tells the compiler that res->addr is not NULL when used with RTE_PTR_ADD().
So it's not an unrelated change.
Multiple places in this patch.


> --- a/drivers/net/intel/fm10k/fm10k.h
> +++ b/drivers/net/intel/fm10k/fm10k.h
> @@ -264,9 +264,9 @@ fm10k_pktmbuf_reset(struct rte_mbuf *mb, uint16_t
> in_port)
>  	mb->nb_segs = 1;
> 
>  	/* enforce 512B alignment on default Rx virtual addresses */
> -	mb->data_off = (uint16_t)(RTE_PTR_ALIGN((char *)mb->buf_addr +
> -			RTE_PKTMBUF_HEADROOM, FM10K_RX_DATABUF_ALIGN)
> -			- (char *)mb->buf_addr);
> +	mb->data_off = (uint16_t)RTE_PTR_DIFF(RTE_PTR_ALIGN((char *)mb-
> >buf_addr +
> +			RTE_PKTMBUF_HEADROOM, FM10K_RX_DATABUF_ALIGN),
> +			(char *)mb->buf_addr);

Is using RTE_PTR_DIFF required here? Or can the code be unchanged?


> diff --git a/drivers/net/intel/fm10k/fm10k_rxtx_vec.c
> b/drivers/net/intel/fm10k/fm10k_rxtx_vec.c
> index 0eada7275e..a08af75bc7 100644
> --- a/drivers/net/intel/fm10k/fm10k_rxtx_vec.c
> +++ b/drivers/net/intel/fm10k/fm10k_rxtx_vec.c
> @@ -315,12 +315,12 @@ fm10k_rxq_rearm(struct fm10k_rx_queue *rxq)
>  		_mm_store_si128(RTE_CAST_PTR(__m128i *, &rxdp++->q),
> dma_addr1);
> 
>  		/* enforce 512B alignment on default Rx virtual addresses
> */
> -		mb0->data_off = (uint16_t)(RTE_PTR_ALIGN((char *)mb0-
> >buf_addr
> -				+ RTE_PKTMBUF_HEADROOM, FM10K_RX_DATABUF_ALIGN)
> -				- (char *)mb0->buf_addr);
> -		mb1->data_off = (uint16_t)(RTE_PTR_ALIGN((char *)mb1-
> >buf_addr
> -				+ RTE_PKTMBUF_HEADROOM, FM10K_RX_DATABUF_ALIGN)
> -				- (char *)mb1->buf_addr);
> +		mb0->data_off = (uint16_t)RTE_PTR_DIFF(RTE_PTR_ALIGN((char
> *)mb0->buf_addr
> +				+ RTE_PKTMBUF_HEADROOM,
> FM10K_RX_DATABUF_ALIGN),
> +				(char *)mb0->buf_addr);
> +		mb1->data_off = (uint16_t)RTE_PTR_DIFF(RTE_PTR_ALIGN((char
> *)mb1->buf_addr
> +				+ RTE_PKTMBUF_HEADROOM,
> FM10K_RX_DATABUF_ALIGN),
> +				(char *)mb1->buf_addr);

Same: Is using RTE_PTR_DIFF required here?

> --- a/drivers/net/mlx4/mlx4_txq.c
> +++ b/drivers/net/mlx4/mlx4_txq.c
> @@ -114,7 +114,8 @@ txq_uar_uninit_secondary(struct txq *txq)
>  	void *addr;
> 
>  	addr = ppriv->uar_table[txq->stats.idx];
> -	munmap(RTE_PTR_ALIGN_FLOOR(addr, page_size), page_size);
> +	if (addr)

Correction: if (addr != NULL)

> +		munmap(RTE_PTR_ALIGN_FLOOR(addr, page_size), page_size);
>  }
> 


> --- a/lib/mbuf/rte_mbuf.c
> +++ b/lib/mbuf/rte_mbuf.c
> @@ -193,6 +193,7 @@ __rte_pktmbuf_init_extmem(struct rte_mempool *mp,
> 
>  	RTE_ASSERT(ctx->ext < ctx->ext_num);
>  	RTE_ASSERT(ctx->off + ext_mem->elt_size <= ext_mem->buf_len);
> +	RTE_ASSERT(ext_mem->buf_ptr);
> 
>  	m->buf_addr = RTE_PTR_ADD(ext_mem->buf_ptr, ctx->off);

Correction: RTE_ASSERT(ext_mem->buf_ptr != NULL);

Do we also need __rte_assume(ext_mem->buf_ptr != NULL) for when building without RTE_ENABLE_ASSERT?
If so, remember any other RTE_ASSERT()s ensuring non-NULL for RTE_PTR_ADD().


> diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
> index 3042d94c14..f1ff668205 100644
> --- a/lib/mempool/rte_mempool.c
> +++ b/lib/mempool/rte_mempool.c
> @@ -349,9 +349,9 @@ rte_mempool_populate_iova(struct rte_mempool *mp,
> char *vaddr,
>  	memhdr->opaque = opaque;
> 
>  	if (mp->flags & RTE_MEMPOOL_F_NO_CACHE_ALIGN)
> -		off = RTE_PTR_ALIGN_CEIL(vaddr, 8) - vaddr;
> +		off = RTE_PTR_DIFF(RTE_PTR_ALIGN_CEIL(vaddr, 8), vaddr);
>  	else
> -		off = RTE_PTR_ALIGN_CEIL(vaddr, RTE_MEMPOOL_ALIGN) - vaddr;
> +		off = RTE_PTR_DIFF(RTE_PTR_ALIGN_CEIL(vaddr,
> RTE_MEMPOOL_ALIGN), vaddr);

Same: Is using RTE_PTR_DIFF required here?

> 
>  	if (off > len) {
>  		ret = 0;
> @@ -425,8 +425,8 @@ rte_mempool_populate_virt(struct rte_mempool *mp,
> char *addr,
> 
>  		/* populate with the largest group of contiguous pages */
>  		for (phys_len = RTE_MIN(
> -			(size_t)(RTE_PTR_ALIGN_CEIL(addr + off + 1, pg_sz) -
> -				(addr + off)),
> +			(size_t)RTE_PTR_DIFF(RTE_PTR_ALIGN_CEIL(addr + off +
> 1, pg_sz),
> +				addr + off),

Same: Is using RTE_PTR_DIFF required here?


> --- a/lib/mempool/rte_mempool_ops_default.c
> +++ b/lib/mempool/rte_mempool_ops_default.c
> @@ -117,7 +117,7 @@ rte_mempool_op_populate_helper(struct rte_mempool
> *mp, unsigned int flags,
>  	for (i = 0; i < max_objs; i++) {
>  		/* avoid objects to cross page boundaries */
>  		if (check_obj_bounds(va + off, pg_sz, total_elt_sz) < 0) {
> -			off += RTE_PTR_ALIGN_CEIL(va + off, pg_sz) - (va +
> off);
> +			off += RTE_PTR_DIFF(RTE_PTR_ALIGN_CEIL(va + off,
> pg_sz), va + off);

Same: Is using RTE_PTR_DIFF required here?