RE: [PATCH v17 2/2] eal: RTE_PTR_ADD/SUB API improvements

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

> RTE_PTR_ADD and RTE_PTR_SUB APIs have a few limitations:
> 1. ptr cast to uintptr_t drops pointer provenance and
> prevents compiler optimizations
> 2. return cast discards qualifiers (const, volatile)
> which may hide correctness/concurrency issues.
> 3. Accepts both "pointers" and "integers as pointers" which
> overloads the use case and constrains the implementation
> to address other challenges.
> 
> This patch splits the API on two dimensions:
> 1. pointer types

Again, thank you for all the work you put into this, Scott!

> 2. integer types that represent pointers

Please get rid of all the RTE_INT_PTR macros.
IMO, these macros look too much like plain wrappers around simple +/- operators.
It seems they are only needed for the cnxk drivers; those drivers can probably be fixed in some other way.

> 
> This split allows addressing each of the challenges above
> and provides distinct APIs for the distinct use cases.
> Examples:
> 1. Clang is able to optimize and improve __rte_raw_cksum
> (which uses RTE_PTR_ADD) by ~40% (100 bytes) to ~8x (1.5k bytes)
> TSC cycles/byte.
> 2. Refactoring discovered cases that dropped qualifiers (volatile)
> that the new API exposes.
> 
> Signed-off-by: Scott Mitchell <scott.k.mitch1@gmail.com>
> ---

A few minor details follow.

>  static inline struct rte_mbuf *
>  rte_mbuf_from_indirect(struct rte_mbuf *mi)
>  {
> +	RTE_ASSERT(mi);

Preferred: RTE_ASSERT(mi != NULL);

>  	return (struct rte_mbuf *)RTE_PTR_SUB(mi->buf_addr, sizeof(*mi) +
> mi->priv_size);
>  }
> 
> @@ -289,6 +290,7 @@ rte_mbuf_to_baddr(struct rte_mbuf *md)
>  static inline void *
>  rte_mbuf_to_priv(struct rte_mbuf *m)
>  {
> +	RTE_ASSERT(m);

Preferred: RTE_ASSERT(m != NULL);

>  	return RTE_PTR_ADD(m, sizeof(struct rte_mbuf));
>  }
> 


> --- a/lib/mempool/rte_mempool.h
> +++ b/lib/mempool/rte_mempool.h
> @@ -376,6 +376,7 @@ struct __rte_cache_aligned rte_mempool {
>  static inline struct rte_mempool_objhdr *
>  rte_mempool_get_header(void *obj)
>  {
> +	RTE_ASSERT(obj);

Preferred: RTE_ASSERT(obj != NULL);

>  	return (struct rte_mempool_objhdr *)RTE_PTR_SUB(obj,
>  		sizeof(struct rte_mempool_objhdr));
>  }
> @@ -399,6 +400,7 @@ static inline struct rte_mempool
> *rte_mempool_from_obj(void *obj)
>  static inline struct rte_mempool_objtlr *rte_mempool_get_trailer(void
> *obj)
>  {
>  	struct rte_mempool *mp = rte_mempool_from_obj(obj);
> +	RTE_ASSERT(mp);

Preferred: RTE_ASSERT(mp != NULL);

>  	return (struct rte_mempool_objtlr *)RTE_PTR_ADD(obj, mp-
> >elt_size);
>  }
> 
> @@ -1844,6 +1846,7 @@ rte_mempool_empty(const struct rte_mempool *mp)
>  static inline rte_iova_t
>  rte_mempool_virt2iova(const void *elt)
>  {
> +	RTE_ASSERT(elt);
>  	const struct rte_mempool_objhdr *hdr;

Preferred: RTE_ASSERT(elt != NULL);
Also, it might be preferable adding the RTE_ASSERT() here instead of above.

>  	hdr = (const struct rte_mempool_objhdr *)RTE_PTR_SUB(elt,
>  		sizeof(*hdr));