[dpdk-dev] rte_mbuf library likely()/unlikely()

[dpdk-dev] rte_mbuf library likely()/unlikely()

[Morten Brørup]

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");

 

        /* generic checks */

-       if (m->pool == NULL)

+       if (unlikely(m->pool == NULL))

                rte_panic("bad mbuf pool\n");

-       if (m->buf_iova == 0)

+       if (unlikely(m->buf_iova == 0))

                rte_panic("bad IO addr\n");

-       if (m->buf_addr == NULL)

+       if (unlikely(m->buf_addr == NULL))

                rte_panic("bad virt addr\n");

 

        uint16_t cnt = rte_mbuf_refcnt_read(m);

-       if ((cnt == 0) || (cnt == UINT16_MAX))

+       if (unlikely((cnt == 0) || (cnt == UINT16_MAX)))

                rte_panic("bad ref cnt\n");

 

        /* nothing to check for sub-segments */

@@ -193,7 +193,7 @@

                return;

 

        /* data_len is supposed to be not more than pkt_len */

-       if (m->data_len > m->pkt_len)

+       if (unlikely(m->data_len > m->pkt_len))

                rte_panic("bad data_len\n");

 

        nb_segs = m->nb_segs;

@@ -204,9 +204,9 @@

                pkt_len -= m->data_len;

        } while ((m = m->next) != NULL);

 

-       if (nb_segs)

+       if (unlikely(nb_segs))

                rte_panic("bad nb_segs\n");

-       if (pkt_len)

+       if (unlikely(pkt_len))

                rte_panic("bad pkt_len\n");

}

 

@@ -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;

 

 

diff -bu rte_mbuf.h.orig rte_mbuf.h

--- rte_mbuf.h.orig     2018-07-23 15:13:26.000000000 +0200

+++ rte_mbuf.h  2018-07-23 15:24:25.000000000 +0200

@@ -1007,7 +1007,7 @@

{

        struct rte_mbuf *m;

 

-       if (rte_mempool_get(mp, (void **)&m) < 0)

+       if (unlikely(rte_mempool_get(mp, (void **)&m) < 0))

                return NULL;

        MBUF_RAW_ALLOC_CHECK(m);

        return m;

@@ -1268,7 +1268,7 @@

static inline struct rte_mbuf *rte_pktmbuf_alloc(struct rte_mempool *mp)

{

        struct rte_mbuf *m;

-       if ((m = rte_mbuf_raw_alloc(mp)) != NULL)

+       if (likely((m = rte_mbuf_raw_alloc(mp)) != NULL))

                rte_pktmbuf_reset(m);

        return m;

}

@@ -1696,7 +1696,7 @@

{

        struct rte_mbuf *m_next;

 

-       if (m != NULL)

+       if (likely(m != NULL))

                __rte_mbuf_sanity_check(m, 1);

 

        while (m != NULL) {

@@ -2099,7 +2099,7 @@

        struct rte_mbuf *cur_tail;

 

        /* Check for number-of-segments-overflow */

-       if (head->nb_segs + tail->nb_segs > RTE_MBUF_MAX_NB_SEGS)

+       if (unlikely(head->nb_segs + tail->nb_segs > RTE_MBUF_MAX_NB_SEGS))

                return -EOVERFLOW;

 

        /* Chain 'tail' onto the old tail */

@@ -2147,28 +2147,28 @@

                                  m->outer_l3_len;

 

        /* Headers are fragmented */

-       if (rte_pktmbuf_data_len(m) < inner_l3_offset + m->l3_len + m->l4_len)

+       if (unlikely(rte_pktmbuf_data_len(m) < inner_l3_offset + m->l3_len + m->l4_len))

                return -ENOTSUP;

 

        /* IP checksum can be counted only for IPv4 packet */

-       if ((ol_flags & PKT_TX_IP_CKSUM) && (ol_flags & PKT_TX_IPV6))

+       if (unlikely((ol_flags & PKT_TX_IP_CKSUM) && (ol_flags & PKT_TX_IPV6)))

                return -EINVAL;

 

        /* IP type not set when required */

        if (ol_flags & (PKT_TX_L4_MASK | PKT_TX_TCP_SEG))

-               if (!(ol_flags & (PKT_TX_IPV4 | PKT_TX_IPV6)))

+               if (unlikely(!(ol_flags & (PKT_TX_IPV4 | PKT_TX_IPV6))))

                        return -EINVAL;

 

        /* Check requirements for TSO packet */

        if (ol_flags & PKT_TX_TCP_SEG)

-               if ((m->tso_segsz == 0) ||

+               if (unlikely((m->tso_segsz == 0) ||

                                ((ol_flags & PKT_TX_IPV4) &&

-                               !(ol_flags & PKT_TX_IP_CKSUM)))

+                               !(ol_flags & PKT_TX_IP_CKSUM))))

                        return -EINVAL;

 

        /* PKT_TX_OUTER_IP_CKSUM set for non outer IPv4 packet. */

-       if ((ol_flags & PKT_TX_OUTER_IP_CKSUM) &&

-                       !(ol_flags & PKT_TX_OUTER_IPV4))

+       if (unlikely((ol_flags & PKT_TX_OUTER_IP_CKSUM) &&

+                       !(ol_flags & PKT_TX_OUTER_IPV4)))

                return -EINVAL;

 

        return 0;

 

 

diff -bu rte_mbuf_ptype.c.orig rte_mbuf_ptype.c

--- rte_mbuf_ptype.c.orig       2018-07-23 15:45:49.000000000 +0200

+++ rte_mbuf_ptype.c    2018-07-23 15:44:59.000000000 +0200

@@ -118,15 +118,15 @@

{

        int ret;

 

-       if (buflen == 0)

+       if (unlikely(buflen == 0))

                return -1;

 

        buf[0] = '\0';

        if ((ptype & RTE_PTYPE_ALL_MASK) == RTE_PTYPE_UNKNOWN) {

                ret = snprintf(buf, buflen, "UNKNOWN");

-               if (ret < 0)

+               if (unlikely(ret < 0))

                        return -1;

-               if ((size_t)ret >= buflen)

+               if (unlikely((size_t)ret >= buflen))

                       return -1;

                return 0;

        }

@@ -134,9 +134,9 @@

        if ((ptype & RTE_PTYPE_L2_MASK) != 0) {

                ret = snprintf(buf, buflen, "%s ",

                        rte_get_ptype_l2_name(ptype));

-               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;

@@ -144,9 +144,9 @@

        if ((ptype & RTE_PTYPE_L3_MASK) != 0) {

                ret = snprintf(buf, buflen, "%s ",

                        rte_get_ptype_l3_name(ptype));

-               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;

@@ -154,9 +154,9 @@

        if ((ptype & RTE_PTYPE_L4_MASK) != 0) {

                ret = snprintf(buf, buflen, "%s ",

                        rte_get_ptype_l4_name(ptype));

-               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;

@@ -164,9 +164,9 @@

        if ((ptype & RTE_PTYPE_TUNNEL_MASK) != 0) {

                ret = snprintf(buf, buflen, "%s ",

                        rte_get_ptype_tunnel_name(ptype));

-               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;

@@ -174,9 +174,9 @@

        if ((ptype & RTE_PTYPE_INNER_L2_MASK) != 0) {

                ret = snprintf(buf, buflen, "%s ",

                        rte_get_ptype_inner_l2_name(ptype));

-               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;

@@ -184,9 +184,9 @@

        if ((ptype & RTE_PTYPE_INNER_L3_MASK) != 0) {

                ret = snprintf(buf, buflen, "%s ",

                        rte_get_ptype_inner_l3_name(ptype));

-               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;

@@ -194,9 +194,9 @@

        if ((ptype & RTE_PTYPE_INNER_L4_MASK) != 0) {

                ret = snprintf(buf, buflen, "%s ",

                        rte_get_ptype_inner_l4_name(ptype));

-               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

CTO

 

 

SmartShare Systems A/S

Tonsbakken 16-18

DK-2740 Skovlunde

Denmark

 

Office      +45 70 20 00 93

Direct      +45 89 93 50 22

Mobile     +45 25 40 82 12

 

mb@smartsharesystems.com <mailto:mb@smartsharesystems.com> 

www.smartsharesystems.com <https://www.smartsharesystems.com/> 

 

Re: [dpdk-dev] rte_mbuf library likely()/unlikely()

[Stephen Hemminger]

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.

Re: [dpdk-dev] rte_mbuf library likely()/unlikely()

[Honnappa Nagarahalli]

Do you see any performance improvements with these changes?

-----Original Message-----
From: dev <dev-bounces@dpdk.org> On Behalf Of Morten Brørup
Sent: Monday, July 23, 2018 8:54 AM
To: Olivier Matz <olivier.matz@6wind.com>
Cc: dev@dpdk.org
Subject: [dpdk-dev] rte_mbuf library likely()/unlikely()

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");



        /* generic checks */

-       if (m->pool == NULL)

+       if (unlikely(m->pool == NULL))

                rte_panic("bad mbuf pool\n");

-       if (m->buf_iova == 0)

+       if (unlikely(m->buf_iova == 0))

                rte_panic("bad IO addr\n");

-       if (m->buf_addr == NULL)

+       if (unlikely(m->buf_addr == NULL))

                rte_panic("bad virt addr\n");



        uint16_t cnt = rte_mbuf_refcnt_read(m);

-       if ((cnt == 0) || (cnt == UINT16_MAX))

+       if (unlikely((cnt == 0) || (cnt == UINT16_MAX)))

                rte_panic("bad ref cnt\n");



        /* nothing to check for sub-segments */

@@ -193,7 +193,7 @@

                return;



        /* data_len is supposed to be not more than pkt_len */

-       if (m->data_len > m->pkt_len)

+       if (unlikely(m->data_len > m->pkt_len))

                rte_panic("bad data_len\n");



        nb_segs = m->nb_segs;

@@ -204,9 +204,9 @@

                pkt_len -= m->data_len;

        } while ((m = m->next) != NULL);



-       if (nb_segs)

+       if (unlikely(nb_segs))

                rte_panic("bad nb_segs\n");

-       if (pkt_len)

+       if (unlikely(pkt_len))

                rte_panic("bad pkt_len\n");

}



@@ -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;





diff -bu rte_mbuf.h.orig rte_mbuf.h

--- rte_mbuf.h.orig     2018-07-23 15:13:26.000000000 +0200

+++ rte_mbuf.h  2018-07-23 15:24:25.000000000 +0200

@@ -1007,7 +1007,7 @@

{

        struct rte_mbuf *m;



-       if (rte_mempool_get(mp, (void **)&m) < 0)

+       if (unlikely(rte_mempool_get(mp, (void **)&m) < 0))

                return NULL;

        MBUF_RAW_ALLOC_CHECK(m);

        return m;

@@ -1268,7 +1268,7 @@

static inline struct rte_mbuf *rte_pktmbuf_alloc(struct rte_mempool *mp)

{

        struct rte_mbuf *m;

-       if ((m = rte_mbuf_raw_alloc(mp)) != NULL)

+       if (likely((m = rte_mbuf_raw_alloc(mp)) != NULL))

                rte_pktmbuf_reset(m);

        return m;

}

@@ -1696,7 +1696,7 @@

{

        struct rte_mbuf *m_next;



-       if (m != NULL)

+       if (likely(m != NULL))

                __rte_mbuf_sanity_check(m, 1);



        while (m != NULL) {

@@ -2099,7 +2099,7 @@

        struct rte_mbuf *cur_tail;



        /* Check for number-of-segments-overflow */

-       if (head->nb_segs + tail->nb_segs > RTE_MBUF_MAX_NB_SEGS)

+       if (unlikely(head->nb_segs + tail->nb_segs > RTE_MBUF_MAX_NB_SEGS))

                return -EOVERFLOW;



        /* Chain 'tail' onto the old tail */

@@ -2147,28 +2147,28 @@

                                  m->outer_l3_len;



        /* Headers are fragmented */

-       if (rte_pktmbuf_data_len(m) < inner_l3_offset + m->l3_len + m->l4_len)

+       if (unlikely(rte_pktmbuf_data_len(m) < inner_l3_offset + m->l3_len + m->l4_len))

                return -ENOTSUP;



        /* IP checksum can be counted only for IPv4 packet */

-       if ((ol_flags & PKT_TX_IP_CKSUM) && (ol_flags & PKT_TX_IPV6))

+       if (unlikely((ol_flags & PKT_TX_IP_CKSUM) && (ol_flags & PKT_TX_IPV6)))

                return -EINVAL;



        /* IP type not set when required */

        if (ol_flags & (PKT_TX_L4_MASK | PKT_TX_TCP_SEG))

-               if (!(ol_flags & (PKT_TX_IPV4 | PKT_TX_IPV6)))

+               if (unlikely(!(ol_flags & (PKT_TX_IPV4 | PKT_TX_IPV6))))

                        return -EINVAL;



        /* Check requirements for TSO packet */

        if (ol_flags & PKT_TX_TCP_SEG)

-               if ((m->tso_segsz == 0) ||

+               if (unlikely((m->tso_segsz == 0) ||

                                ((ol_flags & PKT_TX_IPV4) &&

-                               !(ol_flags & PKT_TX_IP_CKSUM)))

+                               !(ol_flags & PKT_TX_IP_CKSUM))))

                        return -EINVAL;



        /* PKT_TX_OUTER_IP_CKSUM set for non outer IPv4 packet. */

-       if ((ol_flags & PKT_TX_OUTER_IP_CKSUM) &&

-                       !(ol_flags & PKT_TX_OUTER_IPV4))

+       if (unlikely((ol_flags & PKT_TX_OUTER_IP_CKSUM) &&

+                       !(ol_flags & PKT_TX_OUTER_IPV4)))

                return -EINVAL;



        return 0;





diff -bu rte_mbuf_ptype.c.orig rte_mbuf_ptype.c

--- rte_mbuf_ptype.c.orig       2018-07-23 15:45:49.000000000 +0200

+++ rte_mbuf_ptype.c    2018-07-23 15:44:59.000000000 +0200

@@ -118,15 +118,15 @@

{

        int ret;



-       if (buflen == 0)

+       if (unlikely(buflen == 0))

                return -1;



        buf[0] = '\0';

        if ((ptype & RTE_PTYPE_ALL_MASK) == RTE_PTYPE_UNKNOWN) {

                ret = snprintf(buf, buflen, "UNKNOWN");

-               if (ret < 0)

+               if (unlikely(ret < 0))

                        return -1;

-               if ((size_t)ret >= buflen)

+               if (unlikely((size_t)ret >= buflen))

                       return -1;

                return 0;

        }

@@ -134,9 +134,9 @@

        if ((ptype & RTE_PTYPE_L2_MASK) != 0) {

                ret = snprintf(buf, buflen, "%s ",

                        rte_get_ptype_l2_name(ptype));

-               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;

@@ -144,9 +144,9 @@

        if ((ptype & RTE_PTYPE_L3_MASK) != 0) {

                ret = snprintf(buf, buflen, "%s ",

                        rte_get_ptype_l3_name(ptype));

-               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;

@@ -154,9 +154,9 @@

        if ((ptype & RTE_PTYPE_L4_MASK) != 0) {

                ret = snprintf(buf, buflen, "%s ",

                        rte_get_ptype_l4_name(ptype));

-               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;

@@ -164,9 +164,9 @@

        if ((ptype & RTE_PTYPE_TUNNEL_MASK) != 0) {

                ret = snprintf(buf, buflen, "%s ",

                        rte_get_ptype_tunnel_name(ptype));

-               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;

@@ -174,9 +174,9 @@

        if ((ptype & RTE_PTYPE_INNER_L2_MASK) != 0) {

                ret = snprintf(buf, buflen, "%s ",

                        rte_get_ptype_inner_l2_name(ptype));

-               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;

@@ -184,9 +184,9 @@

        if ((ptype & RTE_PTYPE_INNER_L3_MASK) != 0) {

                ret = snprintf(buf, buflen, "%s ",

                        rte_get_ptype_inner_l3_name(ptype));

-               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;

@@ -194,9 +194,9 @@

        if ((ptype & RTE_PTYPE_INNER_L4_MASK) != 0) {

                ret = snprintf(buf, buflen, "%s ",

                        rte_get_ptype_inner_l4_name(ptype));

-               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

CTO





SmartShare Systems A/S

Tonsbakken 16-18

DK-2740 Skovlunde

Denmark



Office      +45 70 20 00 93

Direct      +45 89 93 50 22

Mobile     +45 25 40 82 12



mb@smartsharesystems.com <mailto:mb@smartsharesystems.com>

www.smartsharesystems.com <https://www.smartsharesystems.com/>



IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [dpdk-dev] rte_mbuf library likely()/unlikely()

[Morten Brørup]

> -----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

Re: [dpdk-dev] rte_mbuf library likely()/unlikely()

[Morten Brørup]

I haven't performance tested, but they are compiler branch prediction hints pointing out the most likely execution path, so I expect them to have a positive effect.

E.g. the first comparison in __rte_pktmbuf_read() is very unlikely to be true - it would mean that the application is trying to read data beyond the packet.

Please also refer to:
https://cellperformance.beyond3d.com/articles/2006/04/branch-patterns-using-gcc.html


> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Honnappa Nagarahalli
> Sent: Monday, July 23, 2018 7:52 PM
> To: Morten Brørup; Olivier Matz
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] rte_mbuf library likely()/unlikely()
> 
> Do you see any performance improvements with these changes?
> 
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Morten Brørup
> Sent: Monday, July 23, 2018 8:54 AM
> To: Olivier Matz <olivier.matz@6wind.com>
> Cc: dev@dpdk.org
> Subject: [dpdk-dev] rte_mbuf library likely()/unlikely()
> 
> 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.
> 

Re: [dpdk-dev] rte_mbuf library likely()/unlikely()

[Stephen Hemminger]

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

Re: [dpdk-dev] rte_mbuf library likely()/unlikely()

[Wiles, Keith]

> On Jul 23, 2018, at 2:09 PM, Morten Brørup <mb@smartsharesystems.com> wrote:
> 
> I haven't performance tested, but they are compiler branch prediction hints pointing out the most likely execution path, so I expect them to have a positive effect.

We really need to make sure this provides any performance improvement and that means it needs to be tested on a number of systems. Can you please do some performance testing or see if we can get the guys doing DPDK performance testing to first give this a try? This area is very sensitive to tweaking.

> 
> E.g. the first comparison in __rte_pktmbuf_read() is very unlikely to be true - it would mean that the application is trying to read data beyond the packet.
> 
> Please also refer to:
> https://cellperformance.beyond3d.com/articles/2006/04/branch-patterns-using-gcc.html
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Honnappa Nagarahalli
>> Sent: Monday, July 23, 2018 7:52 PM
>> To: Morten Brørup; Olivier Matz
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] rte_mbuf library likely()/unlikely()
>> 
>> Do you see any performance improvements with these changes?
>> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Morten Brørup
>> Sent: Monday, July 23, 2018 8:54 AM
>> To: Olivier Matz <olivier.matz@6wind.com>
>> Cc: dev@dpdk.org
>> Subject: [dpdk-dev] rte_mbuf library likely()/unlikely()
>> 
>> 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.
>> 

Regards,
Keith

Re: [dpdk-dev] rte_mbuf library likely()/unlikely()

[Olivier Matz]

Hi,

On Mon, Jul 23, 2018 at 10:40:03PM +0000, Wiles, Keith wrote:
> 
> 
> > On Jul 23, 2018, at 2:09 PM, Morten Brørup <mb@smartsharesystems.com> wrote:
> > 
> > I haven't performance tested, but they are compiler branch prediction hints pointing out the most likely execution path, so I expect them to have a positive effect.
> 
> We really need to make sure this provides any performance improvement and that means it needs to be tested on a number of systems. Can you please do some performance testing or see if we can get the guys doing DPDK performance testing to first give this a try? This area is very sensitive to tweaking.

I agree we should be driven by performance improvements.

I remember a discussion with Bruce on the ML saying that hardware branch
predictors generally do a good job.

Thanks,
Olivier

Re: [dpdk-dev] rte_mbuf library likely()/unlikely()

[Morten Brørup]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> Sent: Tuesday, July 24, 2018 9:29 AM
> 
> Hi,
> 
> On Mon, Jul 23, 2018 at 10:40:03PM +0000, Wiles, Keith wrote:
> >
> >
> > > On Jul 23, 2018, at 2:09 PM, Morten Brørup
> <mb@smartsharesystems.com> wrote:
> > >
> > > I haven't performance tested, but they are compiler branch
> prediction hints pointing out the most likely execution path, so I
> expect them to have a positive effect.
> >
> > We really need to make sure this provides any performance improvement
> and that means it needs to be tested on a number of systems. Can you
> please do some performance testing or see if we can get the guys doing
> DPDK performance testing to first give this a try? This area is very
> sensitive to tweaking.
> 
> I agree we should be driven by performance improvements.

Which is why I suggested these changes. Theoretically, they will provide a performance improvement. The most likely execution path is obvious from code review.

> I remember a discussion with Bruce on the ML saying that hardware
> branch
> predictors generally do a good job.

They do, and it is very well documented. E.g. here's a really interesting historical review about branch predictors:
https://danluu.com/branch-prediction/

However, just because hardware branch predictors are pretty good, I don't think we should remove or stop adding likely()/unlikely() and other branch prediction hints. The hints still add value, both for execution speed and for source code readability.

Please also refer to the other link I provided about GCC branches. It basically says that GCC treats an If-sentence like this:

If (Condition) Then
	Expect to execute this
Else
	Do not expect to execute this

So if we don't want unlikely() around an if-condition which probably evaluates to false, we should rewrite the execution order accordingly.

Although hardware branch predictors help a lot most of the time, the likely()/unlikely() still helps the first time the CPU executes the branch instruction.

Furthermore, I'm very well aware of the rule of thumb for adding likely()/unlikely(): Don't add one if it isn't correct almost every time the branch is considered.

How much more compiler branch prediction hints adds to hardware compiler branch prediction is a somewhat academic discussion. But Honnappa and Keith are right: Performance improvements should be performance tested.

Unfortunately, I don't have the equipment or resources to perform a usable performance test, so I submitted the changes to the mailing list for code review instead. And I'm certainly getting code reviewed now. :-)

From a code review perspective, someone else than me might observe that the exception handling execution path is "missing" the unlikely() hint, so I would argue that code readability is an argument for adding it - unless performance testing shows a slowdown.


Med venlig hilsen / kind regards
- Morten Brørup

Re: [dpdk-dev] rte_mbuf library likely()/unlikely()

[Van Haaren, Harry]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Morten Brørup
> Sent: Tuesday, July 24, 2018 9:14 AM
> To: Olivier Matz <olivier.matz@6wind.com>; Wiles, Keith
> <keith.wiles@intel.com>
> Cc: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] rte_mbuf library likely()/unlikely()
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> > Sent: Tuesday, July 24, 2018 9:29 AM
> >
> > Hi,
> >
> > On Mon, Jul 23, 2018 at 10:40:03PM +0000, Wiles, Keith wrote:
> > >
> > >
> > > > On Jul 23, 2018, at 2:09 PM, Morten Brørup
> > <mb@smartsharesystems.com> wrote:
> > > >
> > > > I haven't performance tested, but they are compiler branch
> > prediction hints pointing out the most likely execution path, so I
> > expect them to have a positive effect.
> > >
> > > We really need to make sure this provides any performance improvement
> > and that means it needs to be tested on a number of systems. Can you
> > please do some performance testing or see if we can get the guys doing
> > DPDK performance testing to first give this a try? This area is very
> > sensitive to tweaking.
> >
> > I agree we should be driven by performance improvements.
> 
> Which is why I suggested these changes. Theoretically, they will provide a
> performance improvement. The most likely execution path is obvious from code
> review.


> > I remember a discussion with Bruce on the ML saying that hardware
> > branch predictors generally do a good job.
> 
> They do, and it is very well documented. E.g. here's a really interesting
> historical review about branch predictors:
> https://danluu.com/branch-prediction/
> 
> However, just because hardware branch predictors are pretty good, I don't
> think we should remove or stop adding likely()/unlikely() and other branch
> prediction hints. The hints still add value, both for execution speed and
> for source code readability.


Although "likely" and "unlikely" sound like they predict branch-taken
or branch-not-taken, in practice the UNLIKELY() macro expands to __builtin_expect().

The compiler uses __builtin_expect() to understand what code is expected to
run most of the time. In *practice* what this means is that the code that is
not expected to run gets moved "far away" in the binary itself. The compiler
can layout code to be better with the "static branch prediction" methods (see below).

Think of UNLIKELY() not as helping a runtime branch predictor, it doesn't.
Think of UNLIKELY() as a method to move error-handling instructions to cold
instruction-cache-lines.

To put it another way, using UNLIKELY() puts the unlikely branch code far away,
and there's a chance we're going to pay an i-cache miss for that.

As a summary:
UNLIKELY() for *error handling* is good, it moves cold instructions out of hot i-cache
UNLIKELY() on anything that is valid to happen (e.g. ring dequeue == 0) is not advised,
as we could cause i-cache misses on the data path. 


> Please also refer to the other link I provided about GCC branches. It
> basically says that GCC treats an If-sentence like this:
> 
> If (Condition) Then
> 	Expect to execute this
> Else
> 	Do not expect to execute this
> 
> So if we don't want unlikely() around an if-condition which probably
> evaluates to false, we should rewrite the execution order accordingly.

In the case of error handling, this is correct - and the compiler will handle it.
If both taken and not-taken occur at runtime, let the runtime branch predictor handle it.


> Although hardware branch predictors help a lot most of the time, the
> likely()/unlikely() still helps the first time the CPU executes the branch
> instruction.

There is a static branch prediction scheme that predicts branches that are
executing for the first time. It is detailed in the Software Optimization Manual,
Section 3.4 "Optimizing the front end", Page 103 "Static Branch Prediction Algorithm":

https://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-optimization-manual.pdf

UNLIKELY() tells GCC which layout to use - so it does help somewhat, but the
price of a single i-cache miss is much higher than multiple branch-misses...


> Furthermore, I'm very well aware of the rule of thumb for adding
> likely()/unlikely(): Don't add one if it isn't correct almost every time the
> branch is considered.

I would word this stronger: if both taken and not-taken are *valid*, don't use UNLIKELY().
Use UNLIKELY() for error handling cases only.


> How much more compiler branch prediction hints adds to hardware compiler
> branch prediction is a somewhat academic discussion. But Honnappa and Keith
> are right: Performance improvements should be performance tested.

+1 that ultimately it comes down to measured numbers, on a variety of platforms..

..but micro-benchmarks are not enough either. What if the whole micro-bench fits into
i-cache, it looks like there is no difference in perf. Then apply these UNLIKELY()
changes to a real world app, and the i-cache misses may become much more expensive.


> Unfortunately, I don't have the equipment or resources to perform a usable
> performance test, so I submitted the changes to the mailing list for code
> review instead. And I'm certainly getting code reviewed now. :-)
> 
> From a code review perspective, someone else than me might observe that the
> exception handling execution path is "missing" the unlikely() hint, so I
> would argue that code readability is an argument for adding it - unless
> performance testing shows a slowdown.
> 
> 
> Med venlig hilsen / kind regards
> - Morten Brørup

Unless you see a significant branch-miss bottleneck somewhere in the code,
my opinion is that we are trying to micro-optimize code, when there are probably
larger problems to solve.

Re: [dpdk-dev] rte_mbuf library likely()/unlikely()

[Wiles, Keith]

> On Jul 24, 2018, at 6:31 AM, Van Haaren, Harry <harry.van.haaren@intel.com> wrote:
> 
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Morten Brørup
>> Sent: Tuesday, July 24, 2018 9:14 AM
>> To: Olivier Matz <olivier.matz@6wind.com>; Wiles, Keith
>> <keith.wiles@intel.com>
>> Cc: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] rte_mbuf library likely()/unlikely()
>> 
>>> -----Original Message-----
>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
>>> Sent: Tuesday, July 24, 2018 9:29 AM
>>> 
>>> Hi,
>>> 
>>> On Mon, Jul 23, 2018 at 10:40:03PM +0000, Wiles, Keith wrote:
>>>> 
>>>> 
>>>>> On Jul 23, 2018, at 2:09 PM, Morten Brørup
>>> <mb@smartsharesystems.com> wrote:
>>>>> 
>>>>> I haven't performance tested, but they are compiler branch
>>> prediction hints pointing out the most likely execution path, so I
>>> expect them to have a positive effect.
>>>> 
>>>> We really need to make sure this provides any performance improvement
>>> and that means it needs to be tested on a number of systems. Can you
>>> please do some performance testing or see if we can get the guys doing
>>> DPDK performance testing to first give this a try? This area is very
>>> sensitive to tweaking.
>>> 
>>> I agree we should be driven by performance improvements.
>> 
>> Which is why I suggested these changes. Theoretically, they will provide a
>> performance improvement. The most likely execution path is obvious from code
>> review.
> 
> 
>>> I remember a discussion with Bruce on the ML saying that hardware
>>> branch predictors generally do a good job.
>> 
>> They do, and it is very well documented. E.g. here's a really interesting
>> historical review about branch predictors:
>> https://danluu.com/branch-prediction/
>> 
>> However, just because hardware branch predictors are pretty good, I don't
>> think we should remove or stop adding likely()/unlikely() and other branch
>> prediction hints. The hints still add value, both for execution speed and
>> for source code readability.
> 
> 
> Although "likely" and "unlikely" sound like they predict branch-taken
> or branch-not-taken, in practice the UNLIKELY() macro expands to __builtin_expect().
> 
> The compiler uses __builtin_expect() to understand what code is expected to
> run most of the time. In *practice* what this means is that the code that is
> not expected to run gets moved "far away" in the binary itself. The compiler
> can layout code to be better with the "static branch prediction" methods (see below).
> 
> Think of UNLIKELY() not as helping a runtime branch predictor, it doesn't.
> Think of UNLIKELY() as a method to move error-handling instructions to cold
> instruction-cache-lines.
> 
> To put it another way, using UNLIKELY() puts the unlikely branch code far away,
> and there's a chance we're going to pay an i-cache miss for that.
> 
> As a summary:
> UNLIKELY() for *error handling* is good, it moves cold instructions out of hot i-cache
> UNLIKELY() on anything that is valid to happen (e.g. ring dequeue == 0) is not advised,
> as we could cause i-cache misses on the data path. 
> 
> 
>> Please also refer to the other link I provided about GCC branches. It
>> basically says that GCC treats an If-sentence like this:
>> 
>> If (Condition) Then
>> 	Expect to execute this
>> Else
>> 	Do not expect to execute this
>> 
>> So if we don't want unlikely() around an if-condition which probably
>> evaluates to false, we should rewrite the execution order accordingly.
> 
> In the case of error handling, this is correct - and the compiler will handle it.
> If both taken and not-taken occur at runtime, let the runtime branch predictor handle it.
> 
> 
>> Although hardware branch predictors help a lot most of the time, the
>> likely()/unlikely() still helps the first time the CPU executes the branch
>> instruction.
> 
> There is a static branch prediction scheme that predicts branches that are
> executing for the first time. It is detailed in the Software Optimization Manual,
> Section 3.4 "Optimizing the front end", Page 103 "Static Branch Prediction Algorithm":
> 
> https://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-optimization-manual.pdf
> 
> UNLIKELY() tells GCC which layout to use - so it does help somewhat, but the
> price of a single i-cache miss is much higher than multiple branch-misses...
> 
> 
>> Furthermore, I'm very well aware of the rule of thumb for adding
>> likely()/unlikely(): Don't add one if it isn't correct almost every time the
>> branch is considered.
> 
> I would word this stronger: if both taken and not-taken are *valid*, don't use UNLIKELY().
> Use UNLIKELY() for error handling cases only.
> 
> 
>> How much more compiler branch prediction hints adds to hardware compiler
>> branch prediction is a somewhat academic discussion. But Honnappa and Keith
>> are right: Performance improvements should be performance tested.
> 
> +1 that ultimately it comes down to measured numbers, on a variety of platforms..
> 
> ..but micro-benchmarks are not enough either. What if the whole micro-bench fits into
> i-cache, it looks like there is no difference in perf. Then apply these UNLIKELY()
> changes to a real world app, and the i-cache misses may become much more expensive.
> 
> 
>> Unfortunately, I don't have the equipment or resources to perform a usable
>> performance test, so I submitted the changes to the mailing list for code
>> review instead. And I'm certainly getting code reviewed now. :-)
>> 
>> From a code review perspective, someone else than me might observe that the
>> exception handling execution path is "missing" the unlikely() hint, so I
>> would argue that code readability is an argument for adding it - unless
>> performance testing shows a slowdown.
>> 
>> 
>> Med venlig hilsen / kind regards
>> - Morten Brørup
> 
> Unless you see a significant branch-miss bottleneck somewhere in the code,
> my opinion is that we are trying to micro-optimize code, when there are probably
> larger problems to solve.

+1

Regards,
Keith