Re: rte_mbuf library likely()/unlikely()

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 23 Jul 2018 20:59:29 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > -----Original Message-----
> > From: Stephen Hemminger [mailto:stephen@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 <mb@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