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

[Harish Patil <harish.patil@qlogic.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
>

I agree too, lets leave it that way. As I said its optional, to be same as
linux base driver which has:
3809         /* set flag according to packet type (UNICAST_ADDRESS is
default)*/
   3810         if (unlikely(is_multicast_ether_addr(eth->h_dest))) {



Thanks,
Harish


________________________________

This message and any attached documents contain information from the sending company or its parent company(s), subsidiaries, divisions or branch offices that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.