RE: [PATCH v2] mbuf: fix read packet data

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

> > diff --git a/lib/mbuf/rte_mbuf.c b/lib/mbuf/rte_mbuf.c
> > index a5d16e4c97..c2476e7704 100644
> > --- a/lib/mbuf/rte_mbuf.c
> > +++ b/lib/mbuf/rte_mbuf.c
> > @@ -795,7 +795,7 @@ const void *__rte_pktmbuf_read(const struct
> rte_mbuf
> > *m, uint32_t off,
> >  	const struct rte_mbuf *seg = m;
> >  	uint32_t buf_off = 0, copy_len;
> >
> > -	if (off + len > rte_pktmbuf_pkt_len(m))
> > +	if ((uint64_t)off + len > rte_pktmbuf_pkt_len(m))
> 
> Thanks Morten for addressing it.
> Just as a nit, for 32-bit systems, wouldn't it be more friendly:
> If (off + len < off || off + len > rte_pktmbuf_pkt_len(m))
> ?
> Same for comparison in rte_pktmbuf_read().

Neat trick.
I think expanding to 64 bit is easier readable, so I'd rather stick with that.

Also, while the trick is probably faster for 32 bit, there is a risk it could be slower for 64 bit, where I expect expanding to 64 bit has near zero cost.
We could solve that by adding #ifdefs for 32/64 bit architectures, but I think many people in the community would consider it clutter and object to it.

> With or without suggested change:
> Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>

Thanks, Konstantin. I'll keep it as is.

> 
> >  		return NULL;
> >
> >  	while (off >= rte_pktmbuf_data_len(seg)) {
> > diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
> > index 592af2388c..d6602f74bc 100644
> > --- a/lib/mbuf/rte_mbuf.h
> > +++ b/lib/mbuf/rte_mbuf.h
> > @@ -1843,7 +1843,7 @@ const void *__rte_pktmbuf_read(const struct
> rte_mbuf
> > *m, uint32_t off,
> >  static inline const void *rte_pktmbuf_read(const struct rte_mbuf *m,
> >  	uint32_t off, uint32_t len, void *buf)
> >  {
> > -	if (likely(off + len <= rte_pktmbuf_data_len(m)))
> > +	if (likely((uint64_t)off + len <= rte_pktmbuf_data_len(m)))
> >  		return rte_pktmbuf_mtod_offset(m, char *, off);
> >  	else
> >  		return __rte_pktmbuf_read(m, off, len, buf);
> > --
> > 2.43.0