bonding: use ether_addr_equal_unaligned for bond addr compare

PATCH RESEND In review Ding Tianhong 2 participants 9 messages Jan 2, 2014

Subsystems (3)

Networking Network drivers Bonding driver

lore

Share & tools

lore

Ding Tianhong

author

2014-01-02 01:12 UTC #0 lore raw

PATCH RESEND 1/7 bonding: use ether_addr_equal_unaligned for bond addr compare

Use possibly more efficient ether_addr_equal_64bits to instead of memcmp.

Modify the MAC_ADDR_COMPARE to MAC_ADDR_EQUAL, this looks more appropriate.

The comments for the bond 3ad is too old, cleanup some errors and warming.

Signed-off-by Ding Tianhong <[email protected]>

---
 drivers/net/bonding/bond_3ad.c | 62 ++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 29 deletions(-)

drivers/net/bonding/bond_3ad.c +33 ~~-29~~

index 58c2249..5393e1e 100644
@@ -90,8 +90,9 @@
 #define     AD_LINK_SPEED_BITMASK_10000MBPS   0x10
 //endalloun
 
-// compare MAC addresses
-#define MAC_ADDRESS_COMPARE(A, B) memcmp(A, B, ETH_ALEN)
+/* compare MAC addresses */
+#define MAC_ADDRESS_EQUAL(A, B)	\
+	ether_addr_equal_64bits((const u8 *)A, (const u8 *)B)
 
 static struct mac_addr null_mac_addr = { { 0, 0, 0, 0, 0, 0 } };
 static u16 ad_ticks_per_sec;

@@ -417,17 +418,18 @@ static u16 __ad_timer_to_ticks(u16 timer_type, u16 par)
  */
 static void __choose_matched(struct lacpdu *lacpdu, struct port *port)
 {
-	// check if all parameters are alike
+	/* check if all parameters are alike
+	 * or this is individual link(aggregation == FALSE)
+	 * then update the state machine Matched variable.
+	 */
 	if (((ntohs(lacpdu->partner_port) == port->actor_port_number) &&
 	     (ntohs(lacpdu->partner_port_priority) == port->actor_port_priority) &&
-	     !MAC_ADDRESS_COMPARE(&(lacpdu->partner_system), &(port->actor_system)) &&
+	     MAC_ADDRESS_EQUAL(&(lacpdu->partner_system), &(port->actor_system)) &&
 	     (ntohs(lacpdu->partner_system_priority) == port->actor_system_priority) &&
 	     (ntohs(lacpdu->partner_key) == port->actor_oper_port_key) &&
 	     ((lacpdu->partner_state & AD_STATE_AGGREGATION) == (port->actor_oper_port_state & AD_STATE_AGGREGATION))) ||
-	    // or this is individual link(aggregation == FALSE)
 	    ((lacpdu->actor_state & AD_STATE_AGGREGATION) == 0)
 		) {
-		// update the state machine Matched variable
 		port->sm_vars |= AD_PORT_MATCHED;
 	} else {
 		port->sm_vars &= ~AD_PORT_MATCHED;

@@ -507,14 +509,15 @@ static void __update_selected(struct lacpdu *lacpdu, struct port *port)
 	if (lacpdu && port) {
 		const struct port_params *partner = &port->partner_oper;
 
-		// check if any parameter is different
+		/* check if any parameter is different then
+		 * update the state machine selected variable.
+		 */
 		if (ntohs(lacpdu->actor_port) != partner->port_number ||
 		    ntohs(lacpdu->actor_port_priority) != partner->port_priority ||
-		    MAC_ADDRESS_COMPARE(&lacpdu->actor_system, &partner->system) ||
+		    !MAC_ADDRESS_EQUAL(&lacpdu->actor_system, &partner->system) ||
 		    ntohs(lacpdu->actor_system_priority) != partner->system_priority ||
 		    ntohs(lacpdu->actor_key) != partner->key ||
 		    (lacpdu->actor_state & AD_STATE_AGGREGATION) != (partner->port_state & AD_STATE_AGGREGATION)) {
-			// update the state machine Selected variable
 			port->sm_vars &= ~AD_PORT_SELECTED;
 		}
 	}

@@ -538,15 +541,16 @@ static void __update_default_selected(struct port *port)
 		const struct port_params *admin = &port->partner_admin;
 		const struct port_params *oper = &port->partner_oper;
 
-		// check if any parameter is different
+		/* check if any parameter is different then
+		 * update the state machine selected variable.
+		 */
 		if (admin->port_number != oper->port_number ||
 		    admin->port_priority != oper->port_priority ||
-		    MAC_ADDRESS_COMPARE(&admin->system, &oper->system) ||
+		    !MAC_ADDRESS_EQUAL(&admin->system, &oper->system) ||
 		    admin->system_priority != oper->system_priority ||
 		    admin->key != oper->key ||
 		    (admin->port_state & AD_STATE_AGGREGATION)
 			!= (oper->port_state & AD_STATE_AGGREGATION)) {
-			// update the state machine Selected variable
 			port->sm_vars &= ~AD_PORT_SELECTED;
 		}
 	}

@@ -566,12 +570,14 @@ static void __update_default_selected(struct port *port)
  */
 static void __update_ntt(struct lacpdu *lacpdu, struct port *port)
 {
-	// validate lacpdu and port
+	/* validate lacpdu and port */
 	if (lacpdu && port) {
-		// check if any parameter is different
+		/* check if any parameter is different then
+		 * update the port->ntt.
+		 */
 		if ((ntohs(lacpdu->partner_port) != port->actor_port_number) ||
 		    (ntohs(lacpdu->partner_port_priority) != port->actor_port_priority) ||
-		    MAC_ADDRESS_COMPARE(&(lacpdu->partner_system), &(port->actor_system)) ||
+		    !MAC_ADDRESS_EQUAL(&(lacpdu->partner_system), &(port->actor_system)) ||
 		    (ntohs(lacpdu->partner_system_priority) != port->actor_system_priority) ||
 		    (ntohs(lacpdu->partner_key) != port->actor_oper_port_key) ||
 		    ((lacpdu->partner_state & AD_STATE_LACP_ACTIVITY) != (port->actor_oper_port_state & AD_STATE_LACP_ACTIVITY)) ||

@@ -579,7 +585,6 @@ static void __update_ntt(struct lacpdu *lacpdu, struct port *port)
 		    ((lacpdu->partner_state & AD_STATE_SYNCHRONIZATION) != (port->actor_oper_port_state & AD_STATE_SYNCHRONIZATION)) ||
 		    ((lacpdu->partner_state & AD_STATE_AGGREGATION) != (port->actor_oper_port_state & AD_STATE_AGGREGATION))
 		   ) {
-
 			port->ntt = true;
 		}
 	}

@@ -1076,9 +1081,8 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
 			port->actor_oper_port_state &= ~AD_STATE_EXPIRED;
 			break;
 		case AD_RX_CURRENT:
-			// detect loopback situation
-			if (!MAC_ADDRESS_COMPARE(&(lacpdu->actor_system), &(port->actor_system))) {
-				// INFO_RECEIVED_LOOPBACK_FRAMES
+			/* detect loopback situation */
+			if (MAC_ADDRESS_EQUAL(&(lacpdu->actor_system), &(port->actor_system))) {
 				pr_err("%s: An illegal loopback occurred on adapter (%s).\n"
 				       "Check the configuration to verify that all adapters are connected to 802.3ad compliant switch ports\n",
 				       port->slave->bond->dev->name, port->slave->dev->name);

@@ -1090,7 +1094,7 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
 			port->sm_rx_timer_counter = __ad_timer_to_ticks(AD_CURRENT_WHILE_TIMER, (u16)(port->actor_oper_port_state & AD_STATE_LACP_TIMEOUT));
 			port->actor_oper_port_state &= ~AD_STATE_EXPIRED;
 			break;
-		default:    //to silence the compiler
+		default:    /* to silence the compiler */
 			break;
 		}
 	}

@@ -1281,17 +1285,17 @@ static void ad_port_selection_logic(struct port *port)
 				free_aggregator = aggregator;
 			continue;
 		}
-		// check if current aggregator suits us
-		if (((aggregator->actor_oper_aggregator_key == port->actor_oper_port_key) && // if all parameters match AND
-		     !MAC_ADDRESS_COMPARE(&(aggregator->partner_system), &(port->partner_oper.system)) &&
+		/* check if current aggregator suits us */
+		if (((aggregator->actor_oper_aggregator_key == port->actor_oper_port_key) && /* if all parameters match AND */
+		     MAC_ADDRESS_EQUAL(&(aggregator->partner_system), &(port->partner_oper.system)) &&
 		     (aggregator->partner_system_priority == port->partner_oper.system_priority) &&
 		     (aggregator->partner_oper_aggregator_key == port->partner_oper.key)
 		    ) &&
-		    ((MAC_ADDRESS_COMPARE(&(port->partner_oper.system), &(null_mac_addr)) && // partner answers
-		      !aggregator->is_individual)  // but is not individual OR
+		    ((!MAC_ADDRESS_EQUAL(&(port->partner_oper.system), &(null_mac_addr)) && /* partner answers */
+		      !aggregator->is_individual)  /* but is not individual OR */
 		    )
 		   ) {
-			// attach to the founded aggregator
+			/* attach to the founded aggregator */
 			port->aggregator = aggregator;
 			port->actor_port_aggregator_identifier =
 				port->aggregator->aggregator_identifier;

@@ -1705,7 +1709,7 @@ static void ad_enable_collecting_distributing(struct port *port)
  */
 static void ad_disable_collecting_distributing(struct port *port)
 {
-	if (port->aggregator && MAC_ADDRESS_COMPARE(&(port->aggregator->partner_system), &(null_mac_addr))) {
+	if (port->aggregator && !MAC_ADDRESS_EQUAL(&(port->aggregator->partner_system), &(null_mac_addr))) {
 		pr_debug("Disabling port %d(LAG %d)\n",
 			 port->actor_port_number,
 			 port->aggregator->aggregator_identifier);

@@ -1826,8 +1830,8 @@ static u16 aggregator_identifier;
  */
 void bond_3ad_initialize(struct bonding *bond, u16 tick_resolution)
 {
-	// check that the bond is not initialized yet
-	if (MAC_ADDRESS_COMPARE(&(BOND_AD_INFO(bond).system.sys_mac_addr),
+	/* check that the bond is not initialized yet */
+	if (!MAC_ADDRESS_EQUAL(&(BOND_AD_INFO(bond).system.sys_mac_addr),
 				bond->dev->dev_addr)) {
 
 		aggregator_identifier = 0;

Signature · 1 line

1.8.0

Julia Lawall

2014-01-02 07:39 UTC #1 lore raw

drivers/net/bonding/bond_3ad.c line 95

Quoted 5 lines

drivers/net/bonding/bond_3ad.c +3 -2

-// compare MAC addresses
-#define MAC_ADDRESS_COMPARE(A, B) memcmp(A, B, ETH_ALEN)
+/* compare MAC addresses */
+#define MAC_ADDRESS_EQUAL(A, B)	\
+	ether_addr_equal_64bits((const u8 *)A, (const u8 *)B)

Are the casts needed?

julia

Ding Tianhong

author

2014-01-02 08:22 UTC #2 lore raw

Quoted 1 line · Julia Lawall

Are the casts needed

Yes, otherwise the warming will report:

/net-next/drivers/net/bonding/bond_3ad.c:427: warning: passing argument 1 of ‘ether_addr_equal_64bits’ from incompatible pointer type

Regards
Ding

Julia Lawall

2014-01-02 08:38 UTC #3 lore raw

Quoted 7 lines · Ding Tianhong

Quoted 1 line · Julia Lawall
Are the casts needed
Yes, otherwise the warming will report:
/net-next/drivers/net/bonding/bond_3ad.c:427: warning: passing argument 1 of ‘ether_addr_equal_64bits’ from incompatible pointer type

Is it necessary for this driver to use a different type from everyone else?

julia

Ding Tianhong

author

2014-01-02 09:00 UTC #4 lore raw

Quoted 6 lines · Julia Lawall

Quoted 4 lines · Ding Tianhong
Quoted 1 line · Julia Lawall
Are the casts needed

Quoted 9 lines

Quoted 3 lines
Yes, otherwise the warming will report:
/net-next/drivers/net/bonding/bond_3ad.c:427: warning: passing argument 1 of ‘ether_addr_equal_64bits’ from incompatible pointer type
Is it necessary for this driver to use a different type from everyone else?
julia

Did you mean the MAC_ADDRESS_EQUAL is excess?
I did not remove it because the codes no need to be changed more and it looks that didn't take any negative effect.

Regards
Ding

Julia Lawall

2014-01-02 09:14 UTC #5 lore raw

Quoted 11 lines · Ding Tianhong

Quoted 10 lines · Julia Lawall
Quoted 7 lines · Ding Tianhong
Quoted 1 line · Julia Lawall
Are the casts needed
Yes, otherwise the warming will report:
/net-next/drivers/net/bonding/bond_3ad.c:427: warning: passing argument 1 of ‘ether_addr_equal_64bits’ from incompatible pointer type

Quoted 7 lines

Quoted 5 lines
Is it necessary for this driver to use a different type from everyone else?
julia
Did you mean the MAC_ADDRESS_EQUAL is excess?
I did not remove it because the codes no need to be changed more and it looks that didn't take any negative effect.

No, I was wondering about the mac_addr type, defined in bond_3ad.h. Other code just has the array inlined into the containing structure.

julia

Ding Tianhong

author

2014-01-02 10:26 UTC #6 lore raw

Quoted 18 lines · Julia Lawall

Quoted 16 lines · Ding Tianhong
Quoted 15 lines · Julia Lawall
Quoted 7 lines · Ding Tianhong
Quoted 1 line · Julia Lawall
Are the casts needed
Yes, otherwise the warming will report:
/net-next/drivers/net/bonding/bond_3ad.c:427: warning: passing argument 1 of ‘ether_addr_equal_64bits’ from incompatible pointer type
Is it necessary for this driver to use a different type from everyone else?
julia

Quoted 8 lines

Quoted 2 lines
Did you mean the MAC_ADDRESS_EQUAL is excess?
I did not remove it because the codes no need to be changed more and it looks that didn't take any negative effect.
No, I was wondering about the mac_addr type, defined in bond_3ad.h. Other code just has the array inlined into the containing structure.
julia

Oh, sorry for mismatch.:)
The code for bond_3ad mode is too old and the use for mac addr is not so comfortable.

I think I need to send a patch to fix the unusual mac addr and make it more reasonable.

Thanks for your opinion.

Regards
Ding

Ding Tianhong

author

2014-01-02 10:38 UTC #7 lore raw

Quoted 19 lines · Ding Tianhong

Quoted 18 lines · Julia Lawall
Quoted 16 lines · Ding Tianhong
Quoted 15 lines · Julia Lawall
Quoted 7 lines · Ding Tianhong
Quoted 1 line · Julia Lawall
Are the casts needed
Yes, otherwise the warming will report:
/net-next/drivers/net/bonding/bond_3ad.c:427: warning: passing argument 1 of ‘ether_addr_equal_64bits’ from incompatible pointer type
Is it necessary for this driver to use a different type from everyone else?
julia

Quoted 8 lines

Quoted 8 lines
Quoted 2 lines
Did you mean the MAC_ADDRESS_EQUAL is excess?
I did not remove it because the codes no need to be changed more and it looks that didn't take any negative effect.
No, I was wondering about the mac_addr type, defined in bond_3ad.h. Other code just has the array inlined into the containing structure.
julia

I reviewed the struct mac_addr again, and feel that even it looks not comfortable, but make the lacp struct more meaning for 3ad, what do you think about it, I think no need to revert them to u8.

Regards
Ding

Quoted 17 lines

Oh, sorry for mismatch.:)
The code for bond_3ad mode is too old and the use for mac addr is not so comfortable.
I think I need to send a patch to fix the unusual mac addr and make it more reasonable.
Thanks for your opinion.
Regards
Ding
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Julia Lawall

2014-01-02 10:42 UTC #8 lore raw

Quoted 26 lines · Ding Tianhong

Quoted 25 lines · Ding Tianhong
Quoted 24 lines · Julia Lawall
Quoted 11 lines · Ding Tianhong
Quoted 10 lines · Julia Lawall
Quoted 7 lines · Ding Tianhong
Quoted 1 line · Julia Lawall
Are the casts needed
Yes, otherwise the warming will report:
/net-next/drivers/net/bonding/bond_3ad.c:427: warning: passing argument 1 of ‘ether_addr_equal_64bits’ from incompatible pointer type
Quoted 7 lines
Quoted 5 lines
Is it necessary for this driver to use a different type from everyone else?
julia
Did you mean the MAC_ADDRESS_EQUAL is excess?
I did not remove it because the codes no need to be changed more and it looks that didn't take any negative effect.
No, I was wondering about the mac_addr type, defined in bond_3ad.h. Other code just has the array inlined into the containing structure.

Quoted 6 lines

Quoted 2 lines
Quoted 2 lines
julia
I reviewed the struct mac_addr again, and feel that even it looks not comfortable, but make the lacp struct more meaning for 3ad, what do you think about it, I think no need to revert them to u8.

Personally, when I see things that are different, I start wondering about why. So if there is no reason for it to be different, I would prefer that it is the same.

Certainly, a mac_addr type is more meaningful than just an array with size ETH_ALEN, or worse an array with size 6. But I am not sure that it is practical to introduce that type everywhere.

In any case, it is not a big issue.

julia