Re: [dpdk-dev] [PATCH] bnx2x: set Ethernet address type during transmit for VF's

[Bruce Richardson <bruce.richardson@intel.com>]

On Tue, Dec 08, 2015 at 09:27:05AM -0500, Charles (Chas) Williams wrote:
> On Mon, 2015-12-07 at 17:29 +0000, Harish Patil wrote:
> > >
> > >On Sun, 2015-12-06 at 23:34 +0000, Harish Patil wrote:
> > >> >
> > >> >The original was always setting unicast.  While here, clean up some
> > >> >other references that also point into the Ethernet header.
> > >> >
> > >> >Signed-off-by: Chas Williams <3chas3@gmail.com>
> > >> >---
> > >> > drivers/net/bnx2x/bnx2x.c     | 23 +++++++++++++++--------
> > >> > drivers/net/bnx2x/ecore_hsi.h |  5 +++--
> > >> > 2 files changed, 18 insertions(+), 10 deletions(-)
> > >> >
> > >> >diff --git a/drivers/net/bnx2x/bnx2x.c b/drivers/net/bnx2x/bnx2x.c
> > >> >index 76444eb..294711f 100644
> > >> >--- a/drivers/net/bnx2x/bnx2x.c
> > >> >+++ b/drivers/net/bnx2x/bnx2x.c
> > >> >@@ -2177,25 +2177,32 @@ int bnx2x_tx_encap(struct bnx2x_tx_queue *txq,
> > >> >struct rte_mbuf **m_head, int m_p
> > >> >               bd_prod = NEXT_TX_BD(bd_prod);
> > >> >               if (IS_VF(sc)) {
> > >> >                       struct eth_tx_parse_bd_e2 *tx_parse_bd;
> > >> >-                      uint8_t *data = rte_pktmbuf_mtod(m0, uint8_t *);
> > >> >+                      const struct ether_hdr *eh =
> > >>rte_pktmbuf_mtod(m0, struct ether_hdr *);
> > >> >+                      uint8_t mac_type = UNICAST_ADDRESS;
> > >> >
> > >> >                       tx_parse_bd =
> > >> >                           &txq->tx_ring[TX_BD(bd_prod,
> > >>txq)].parse_bd_e2;
> > >> >+                      if (is_multicast_ether_addr(&eh->d_addr)) {
> > >>
> > >> Minor comment. unlikely() may be used here to keep it consistent with
> > >>base
> > >> driver.
> > >
> > >It wasn't clear to me that this code path is all that unlikely().
> > 
> > Its an optional comment, unlikely() is because fast path traffic is mostly
> > unicast.
> 
> Multicast traffic isn't all that uncommon.  I also don't see that we gain
> much from branch prediction here regardless.  My understanding is that
> unlikely() should be used for really unlikely situations since the branch
> will not be optimized.

+1 to this. 

Let the cpu branch predictor do the work at run-time picking the
correct leg of the branch. likely()/unlikely() should only ever be used for
things like fatal error conditions where it doesn't matter how optimized the
unlikely branch part is. For normal code where there is no error, don't try and
optimize it using likely()/unlikely() unless you can prove there is a definite
performance improvement by doing so.

/Bruce