On Mon, 23 Jul 2018 20:59:29 +0200 Morten Brørup <m...@smartsharesystems.com> wrote:
> > -----Original Message----- > > From: Stephen Hemminger [mailto:step...@networkplumber.org] > > Sent: Monday, July 23, 2018 7:38 PM > > To: Morten Brørup > > Cc: Olivier Matz; dev@dpdk.org > > Subject: Re: [dpdk-dev] rte_mbuf library likely()/unlikely() > > > > On Mon, 23 Jul 2018 15:53:42 +0200 > > Morten Brørup <m...@smartsharesystems.com> wrote: > > > > > Hi Olivier, > > > > > > > > > > > > I noticed that __rte_pktmbuf_read() could do with an unlikely(), so I > > went through the entire library. Here are my suggested modifications. > > > > > > > > > > > > > > > > > > diff -bu rte_mbuf.c.orig rte_mbuf.c > > > > > > --- rte_mbuf.c.orig 2018-07-23 15:13:22.000000000 +0200 > > > > > > +++ rte_mbuf.c 2018-07-23 15:32:53.000000000 +0200 > > > > > > @@ -173,19 +173,19 @@ > > > > > > { > > > > > > unsigned int nb_segs, pkt_len; > > > > > > > > > > > > - if (m == NULL) > > > > > > + if (unlikely(m == NULL)) > > > > > > rte_panic("mbuf is NULL\n"); > > > > > > > > > > Adding is unlikely is not necessary since rte_panic is marked with cold > > attribute > > which has the same effect. > > I was not aware of this. Although it is not visible from the source code > files using rte_panic(), it probably means we shouldn't as so much as I > thought. Here's an updated patch for rte_mbuf.c, where it is relevant. The > other two suggested patches are unaffected. > > diff -bu rte_mbuf.c.orig rte_mbuf.c > --- rte_mbuf.c.orig 2018-07-23 15:13:22.000000000 +0200 > +++ rte_mbuf.c 2018-07-23 20:52:35.000000000 +0200 > @@ -249,7 +249,7 @@ > const struct rte_mbuf *seg = m; > uint32_t buf_off = 0, copy_len; > > - if (off + len > rte_pktmbuf_pkt_len(m)) > + if (unlikely(off + len > rte_pktmbuf_pkt_len(m))) > return NULL; > > while (off >= rte_pktmbuf_data_len(seg)) { > @@ -257,7 +257,7 @@ > seg = seg->next; > } > > - if (off + len <= rte_pktmbuf_data_len(seg)) > + if (likely(off + len <= rte_pktmbuf_data_len(seg))) > return rte_pktmbuf_mtod_offset(seg, char *, off); > > /* rare case: header is split among several segments */ > @@ -344,7 +344,7 @@ > unsigned int i; > int ret; > > - if (buflen == 0) > + if (unlikely(buflen == 0)) > return -1; > > buf[0] = '\0'; > @@ -355,9 +355,9 @@ > if (name == NULL) > name = rx_flags[i].default_name; > ret = snprintf(buf, buflen, "%s ", name); > - if (ret < 0) > + if (unlikely(ret < 0)) > return -1; > - if ((size_t)ret >= buflen) > + if (unlikely((size_t)ret >= buflen)) > return -1; > buf += ret; > buflen -= ret; > @@ -440,7 +440,7 @@ > unsigned int i; > int ret; > > - if (buflen == 0) > + if (unlikely(buflen == 0)) > return -1; > > buf[0] = '\0'; > @@ -451,9 +451,9 @@ > if (name == NULL) > name = tx_flags[i].default_name; > ret = snprintf(buf, buflen, "%s ", name); > - if (ret < 0) > + if (unlikely(ret < 0)) > return -1; > - if ((size_t)ret >= buflen) > + if (unlikely((size_t)ret >= buflen)) > return -1; > buf += ret; > buflen -= ret; > > > Med venlig hilsen / kind regards > - Morten Brørup Yes, this makes sense. Please format patch with signed-off-by and submit according to the contributing guidelines Creating Patches section. https://doc.dpdk.org/guides/contributing/patches.html
