diff mbox series

[net-next,v3] bonding: 802.3ad replace MAC_ADDRESS_EQUAL with __agg_has_partner

Message ID SI2PR04MB5097BCA8FF2A2F03D9A5A3EEDC5A2@SI2PR04MB5097.apcprd04.prod.outlook.com
State New
Headers
Series [net-next,v3] bonding: 802.3ad replace MAC_ADDRESS_EQUAL with __agg_has_partner |

Commit Message

Jones Syue 薛懷宗 Feb. 26, 2024, 2:24 a.m. UTC 
  Replace macro MAC_ADDRESS_EQUAL() for null_mac_addr checking with inline
function__agg_has_partner(). When MAC_ADDRESS_EQUAL() is verifiying
aggregator's partner mac addr with null_mac_addr, means that seeing if
aggregator has a valid partner or not. Using __agg_has_partner() makes it
more clear to understand.

In ad_port_selection_logic(), since aggregator->partner_system and
port->partner_oper.system has been compared first as a prerequisite, it is
safe to replace the upcoming MAC_ADDRESS_EQUAL() for null_mac_addr checking
with __agg_has_partner().

Delete null_mac_addr, which is not required anymore in bond_3ad.c, since
all references to it are gone.

Signed-off-by: Jones Syue <jonessyue@qnap.com>
---
v3:
  - replace macro with inline function in ad_port_selection_logic()
  - delete static variable null_mac_addr in bond_3ad.c
  - re-phrase patch description with more precise text
  - re-phrase patch description in imperative mood
v2: https://lore.kernel.org/netdev/SI2PR04MB5097AA23EE6799B3E56C0762DC552@SI2PR04MB5097.apcprd04.prod.outlook.com/
  - add correct CC list by 'get_maintainer.pl -f .../bonding.rst'
v1: https://lore.kernel.org/netdev/SI2PR04MB50977DA9BB51D9C8FAF6928ADC562@SI2PR04MB5097.apcprd04.prod.outlook.com/
---
 drivers/net/bonding/bond_3ad.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

Comments

Hangbin Liu Feb. 26, 2024, 10:08 a.m. UTC | #1 
On Mon, Feb 26, 2024 at 02:24:52AM +0000, Jones Syue 薛懷宗 wrote:
> Replace macro MAC_ADDRESS_EQUAL() for null_mac_addr checking with inline
> function__agg_has_partner(). When MAC_ADDRESS_EQUAL() is verifiying

nit: function __agg_has_partner()

> aggregator's partner mac addr with null_mac_addr, means that seeing if
> aggregator has a valid partner or not. Using __agg_has_partner() makes it
> more clear to understand.
> 
> In ad_port_selection_logic(), since aggregator->partner_system and
> port->partner_oper.system has been compared first as a prerequisite, it is
> safe to replace the upcoming MAC_ADDRESS_EQUAL() for null_mac_addr checking
> with __agg_has_partner().
> 
> Delete null_mac_addr, which is not required anymore in bond_3ad.c, since
> all references to it are gone.
> 
> Signed-off-by: Jones Syue <jonessyue@qnap.com>

Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>
Jiri Pirko Feb. 26, 2024, 12:53 p.m. UTC | #2 
Mon, Feb 26, 2024 at 03:24:52AM CET, jonessyue@qnap.com wrote:
>Replace macro MAC_ADDRESS_EQUAL() for null_mac_addr checking with inline
>function__agg_has_partner(). When MAC_ADDRESS_EQUAL() is verifiying
>aggregator's partner mac addr with null_mac_addr, means that seeing if
>aggregator has a valid partner or not. Using __agg_has_partner() makes it
>more clear to understand.
>
>In ad_port_selection_logic(), since aggregator->partner_system and
>port->partner_oper.system has been compared first as a prerequisite, it is
>safe to replace the upcoming MAC_ADDRESS_EQUAL() for null_mac_addr checking
>with __agg_has_partner().
>
>Delete null_mac_addr, which is not required anymore in bond_3ad.c, since
>all references to it are gone.
>
>Signed-off-by: Jones Syue <jonessyue@qnap.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Jay Vosburgh Feb. 26, 2024, 5:45 p.m. UTC | #3 
Jones Syue 薛懷宗 <jonessyue@qnap.com> wrote:

>Replace macro MAC_ADDRESS_EQUAL() for null_mac_addr checking with inline
>function__agg_has_partner(). When MAC_ADDRESS_EQUAL() is verifiying
>aggregator's partner mac addr with null_mac_addr, means that seeing if
>aggregator has a valid partner or not. Using __agg_has_partner() makes it
>more clear to understand.
>
>In ad_port_selection_logic(), since aggregator->partner_system and
>port->partner_oper.system has been compared first as a prerequisite, it is
>safe to replace the upcoming MAC_ADDRESS_EQUAL() for null_mac_addr checking
>with __agg_has_partner().
>
>Delete null_mac_addr, which is not required anymore in bond_3ad.c, since
>all references to it are gone.
>
>Signed-off-by: Jones Syue <jonessyue@qnap.com>
>---
>v3:
>  - replace macro with inline function in ad_port_selection_logic()
>  - delete static variable null_mac_addr in bond_3ad.c
>  - re-phrase patch description with more precise text
>  - re-phrase patch description in imperative mood
>v2: https://lore.kernel.org/netdev/SI2PR04MB5097AA23EE6799B3E56C0762DC552@SI2PR04MB5097.apcprd04.prod.outlook.com/
>  - add correct CC list by 'get_maintainer.pl -f .../bonding.rst'
>v1: https://lore.kernel.org/netdev/SI2PR04MB50977DA9BB51D9C8FAF6928ADC562@SI2PR04MB5097.apcprd04.prod.outlook.com/
>---
> drivers/net/bonding/bond_3ad.c | 14 +++-----------
> 1 file changed, 3 insertions(+), 11 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3adc
>index f2942e8c6c91..c6807e473ab7 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -82,10 +82,6 @@ enum ad_link_speed_type {
> #define MAC_ADDRESS_EQUAL(A, B)	\
> 	ether_addr_equal_64bits((const u8 *)A, (const u8 *)B)
> 
>-static const u8 null_mac_addr[ETH_ALEN + 2] __long_aligned = {
>-	0, 0, 0, 0, 0, 0
>-};
>-
> static const u16 ad_ticks_per_sec = 1000 / AD_TIMER_INTERVAL;
> static const int ad_delta_in_ticks = (AD_TIMER_INTERVAL * HZ) / 1000;
> 
>@@ -1588,7 +1584,7 @@ static void ad_port_selection_logic(struct port *port, bool *update_slave_arr)
> 		     (aggregator->partner_system_priority == port->partner_oper.system_priority) &&
> 		     (aggregator->partner_oper_aggregator_key == port->partner_operkey)
> 		    ) &&
>-		    ((!MAC_ADDRESS_EQUAL(&(port->partner_oper.system), &(null_mac_addr)) && /* partner answers */
>+		    ((__agg_has_partner(aggregator) && /* partner answers */

	I'm not sure this is an equivalent swap, as it is replacing a
test for non-empty of the port's partner MAC with a test of the
aggregator's partner MAC.

	In the port case, it is validating that this specific port has
received a response from its link partner.

	In the aggregator case, it's checking that the aggregator as a
whole has received response from the link partners of members of the
aggregator, which does not include the port under consideration for
inclusion into the aggregator.

	As the port is not yet a member of the aggregator, how is
checking the aggregator's MAC for being non-empty an equivalent test to
the one it replaces?

	-J

> 		      !aggregator->is_individual)  /* but is not individual OR */
> 		    )
> 		   ) {
>@@ -2036,9 +2032,7 @@ static void ad_enable_collecting(struct port *port)
>  */
> static void ad_disable_distributing(struct port *port, bool *update_slave_arr)
> {
>-	if (port->aggregator &&
>-	    !MAC_ADDRESS_EQUAL(&port->aggregator->partner_system,
>-			       &(null_mac_addr))) {
>+	if (port->aggregator && __agg_has_partner(port->aggregator)) {
> 		slave_dbg(port->slave->bond->dev, port->slave->dev,
> 			  "Disabling distributing on port %d (LAG %d)\n",
> 			  port->actor_port_number,
>@@ -2078,9 +2072,7 @@ static void ad_enable_collecting_distributing(struct port *port,
> static void ad_disable_collecting_distributing(struct port *port,
> 					       bool *update_slave_arr)
> {
>-	if (port->aggregator &&
>-	    !MAC_ADDRESS_EQUAL(&(port->aggregator->partner_system),
>-			       &(null_mac_addr))) {
>+	if (port->aggregator && __agg_has_partner(port->aggregator)) {
> 		slave_dbg(port->slave->bond->dev, port->slave->dev,
> 			  "Disabling port %d (LAG %d)\n",
> 			  port->actor_port_number,
>-- 
>2.1.4
>

---
	-Jay Vosburgh, jay.vosburgh@canonical.com
Jay Vosburgh Feb. 26, 2024, 10:47 p.m. UTC | #4 
Jones Syue 薛懷宗 <jonessyue@qnap.com> wrote:

>Replace macro MAC_ADDRESS_EQUAL() for null_mac_addr checking with inline
>function__agg_has_partner(). When MAC_ADDRESS_EQUAL() is verifiying
>aggregator's partner mac addr with null_mac_addr, means that seeing if
>aggregator has a valid partner or not. Using __agg_has_partner() makes it
>more clear to understand.
>
>In ad_port_selection_logic(), since aggregator->partner_system and
>port->partner_oper.system has been compared first as a prerequisite, it is
>safe to replace the upcoming MAC_ADDRESS_EQUAL() for null_mac_addr checking
>with __agg_has_partner().

	Ok, I missed this bit when I read the patch this morning, so you
can ignore my earlier email's question.  Patch looks good to me, glad to
see null_mac_addr get the boot.

>Delete null_mac_addr, which is not required anymore in bond_3ad.c, since
>all references to it are gone.
>
>Signed-off-by: Jones Syue <jonessyue@qnap.com>

Acked-by: Jay Vosburgh <jay.vosburgh@canonical.com>

	-J

>---
>v3:
>  - replace macro with inline function in ad_port_selection_logic()
>  - delete static variable null_mac_addr in bond_3ad.c
>  - re-phrase patch description with more precise text
>  - re-phrase patch description in imperative mood
>v2: https://lore.kernel.org/netdev/SI2PR04MB5097AA23EE6799B3E56C0762DC552@SI2PR04MB5097.apcprd04.prod.outlook.com/
>  - add correct CC list by 'get_maintainer.pl -f .../bonding.rst'
>v1: https://lore.kernel.org/netdev/SI2PR04MB50977DA9BB51D9C8FAF6928ADC562@SI2PR04MB5097.apcprd04.prod.outlook.com/
>---
> drivers/net/bonding/bond_3ad.c | 14 +++-----------
> 1 file changed, 3 insertions(+), 11 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3adc
>index f2942e8c6c91..c6807e473ab7 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -82,10 +82,6 @@ enum ad_link_speed_type {
> #define MAC_ADDRESS_EQUAL(A, B)	\
> 	ether_addr_equal_64bits((const u8 *)A, (const u8 *)B)
> 
>-static const u8 null_mac_addr[ETH_ALEN + 2] __long_aligned = {
>-	0, 0, 0, 0, 0, 0
>-};
>-
> static const u16 ad_ticks_per_sec = 1000 / AD_TIMER_INTERVAL;
> static const int ad_delta_in_ticks = (AD_TIMER_INTERVAL * HZ) / 1000;
> 
>@@ -1588,7 +1584,7 @@ static void ad_port_selection_logic(struct port *port, bool *update_slave_arr)
> 		     (aggregator->partner_system_priority == port->partner_oper.system_priority) &&
> 		     (aggregator->partner_oper_aggregator_key == port->partner_operkey)
> 		    ) &&
>-		    ((!MAC_ADDRESS_EQUAL(&(port->partner_oper.system), &(null_mac_addr)) && /* partner answers */
>+		    ((__agg_has_partner(aggregator) && /* partner answers */
> 		      !aggregator->is_individual)  /* but is not individual OR */
> 		    )
> 		   ) {
>@@ -2036,9 +2032,7 @@ static void ad_enable_collecting(struct port *port)
>  */
> static void ad_disable_distributing(struct port *port, bool *update_slave_arr)
> {
>-	if (port->aggregator &&
>-	    !MAC_ADDRESS_EQUAL(&port->aggregator->partner_system,
>-			       &(null_mac_addr))) {
>+	if (port->aggregator && __agg_has_partner(port->aggregator)) {
> 		slave_dbg(port->slave->bond->dev, port->slave->dev,
> 			  "Disabling distributing on port %d (LAG %d)\n",
> 			  port->actor_port_number,
>@@ -2078,9 +2072,7 @@ static void ad_enable_collecting_distributing(struct port *port,
> static void ad_disable_collecting_distributing(struct port *port,
> 					       bool *update_slave_arr)
> {
>-	if (port->aggregator &&
>-	    !MAC_ADDRESS_EQUAL(&(port->aggregator->partner_system),
>-			       &(null_mac_addr))) {
>+	if (port->aggregator && __agg_has_partner(port->aggregator)) {
> 		slave_dbg(port->slave->bond->dev, port->slave->dev,
> 			  "Disabling port %d (LAG %d)\n",
> 			  port->actor_port_number,
>-- 
>2.1.4
>
Jones Syue 薛懷宗 Feb. 27, 2024, 3:44 p.m. UTC | #5 
Thank you for kind feedback! Hangbin, Jiri, Jakub, and Jay.

>         Ok, I missed this bit when I read the patch this morning, so you
> can ignore my earlier email's question.  Patch looks good to me, glad to
> see null_mac_addr get the boot.
> Acked-by: Jay Vosburgh <jay.vosburgh@canonical.com>

--
Regards,
Jones Syue | 薛懷宗
QNAP Systems, Inc.
patchwork-bot+netdevbpf@kernel.org Feb. 28, 2024, 3:20 a.m. UTC | #6 
Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 26 Feb 2024 02:24:52 +0000 you wrote:
> Replace macro MAC_ADDRESS_EQUAL() for null_mac_addr checking with inline
> function__agg_has_partner(). When MAC_ADDRESS_EQUAL() is verifiying
> aggregator's partner mac addr with null_mac_addr, means that seeing if
> aggregator has a valid partner or not. Using __agg_has_partner() makes it
> more clear to understand.
> 
> In ad_port_selection_logic(), since aggregator->partner_system and
> port->partner_oper.system has been compared first as a prerequisite, it is
> safe to replace the upcoming MAC_ADDRESS_EQUAL() for null_mac_addr checking
> with __agg_has_partner().
> 
> [...]

Here is the summary with links:
  - [net-next,v3] bonding: 802.3ad replace MAC_ADDRESS_EQUAL with __agg_has_partner
    https://git.kernel.org/netdev/net-next/c/4440873f3655

You are awesome, thank you!
diff mbox series

Patch

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index f2942e8c6c91..c6807e473ab7 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -82,10 +82,6 @@  enum ad_link_speed_type {
 #define MAC_ADDRESS_EQUAL(A, B)	\
 	ether_addr_equal_64bits((const u8 *)A, (const u8 *)B)
 
-static const u8 null_mac_addr[ETH_ALEN + 2] __long_aligned = {
-	0, 0, 0, 0, 0, 0
-};
-
 static const u16 ad_ticks_per_sec = 1000 / AD_TIMER_INTERVAL;
 static const int ad_delta_in_ticks = (AD_TIMER_INTERVAL * HZ) / 1000;
 
@@ -1588,7 +1584,7 @@  static void ad_port_selection_logic(struct port *port, bool *update_slave_arr)
 		     (aggregator->partner_system_priority == port->partner_oper.system_priority) &&
 		     (aggregator->partner_oper_aggregator_key == port->partner_oper.key)
 		    ) &&
-		    ((!MAC_ADDRESS_EQUAL(&(port->partner_oper.system), &(null_mac_addr)) && /* partner answers */
+		    ((__agg_has_partner(aggregator) && /* partner answers */
 		      !aggregator->is_individual)  /* but is not individual OR */
 		    )
 		   ) {
@@ -2036,9 +2032,7 @@  static void ad_enable_collecting(struct port *port)
  */
 static void ad_disable_distributing(struct port *port, bool *update_slave_arr)
 {
-	if (port->aggregator &&
-	    !MAC_ADDRESS_EQUAL(&port->aggregator->partner_system,
-			       &(null_mac_addr))) {
+	if (port->aggregator && __agg_has_partner(port->aggregator)) {
 		slave_dbg(port->slave->bond->dev, port->slave->dev,
 			  "Disabling distributing on port %d (LAG %d)\n",
 			  port->actor_port_number,
@@ -2078,9 +2072,7 @@  static void ad_enable_collecting_distributing(struct port *port,
 static void ad_disable_collecting_distributing(struct port *port,
 					       bool *update_slave_arr)
 {
-	if (port->aggregator &&
-	    !MAC_ADDRESS_EQUAL(&(port->aggregator->partner_system),
-			       &(null_mac_addr))) {
+	if (port->aggregator && __agg_has_partner(port->aggregator)) {
 		slave_dbg(port->slave->bond->dev, port->slave->dev,
 			  "Disabling port %d (LAG %d)\n",
 			  port->actor_port_number,