diff mbox series

[v2,net-next,6/8] ionic: flatten calls to ionic_lif_rx_mode

Message ID 20201106001220.68130-7-snelson@pensando.io (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series ionic updates | expand

Commit Message

Shannon Nelson Nov. 6, 2020, 12:12 a.m. UTC
The _ionic_lif_rx_mode() is only used once and really doesn't
need to be broken out.

Signed-off-by: Shannon Nelson <snelson@pensando.io>
---
 .../net/ethernet/pensando/ionic/ionic_lif.c   | 38 ++++++++-----------
 1 file changed, 16 insertions(+), 22 deletions(-)

Comments

Jakub Kicinski Nov. 6, 2020, 5:03 p.m. UTC | #1
On Thu,  5 Nov 2020 16:12:18 -0800 Shannon Nelson wrote:
> +			work = kzalloc(sizeof(*work), GFP_ATOMIC);
> +			if (!work) {
> +				netdev_err(lif->netdev, "%s OOM\n", __func__);
> +				return;
> +			}

Can you drop this message (can be a follow up, since you're just moving
it).

AFAICT ATOMIC doesn't imply NOWARN so the message is redundant no?
Shannon Nelson Nov. 6, 2020, 5:17 p.m. UTC | #2
On 11/6/20 9:03 AM, Jakub Kicinski wrote:
> On Thu,  5 Nov 2020 16:12:18 -0800 Shannon Nelson wrote:
>> +			work = kzalloc(sizeof(*work), GFP_ATOMIC);
>> +			if (!work) {
>> +				netdev_err(lif->netdev, "%s OOM\n", __func__);
>> +				return;
>> +			}
> Can you drop this message (can be a follow up, since you're just moving
> it).
>
> AFAICT ATOMIC doesn't imply NOWARN so the message is redundant no?

Yes, this can probably be cleaned up.  There are several of these left 
over from the very early version of this driver that I'd like to clean 
up, but haven't yet bubbled up high enough on my priority-vs-time list.  
I'll try to get to them in the next week or so.

sln
Saeed Mahameed Nov. 6, 2020, 9:33 p.m. UTC | #3
On Thu, 2020-11-05 at 16:12 -0800, Shannon Nelson wrote:
> The _ionic_lif_rx_mode() is only used once and really doesn't
> need to be broken out.
> 
> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> ---
>  .../net/ethernet/pensando/ionic/ionic_lif.c   | 38 ++++++++---------
> --
>  1 file changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> index a0d26fe4cbc3..ef092ee33e59 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> @@ -1129,29 +1129,10 @@ static void ionic_lif_rx_mode(struct
> ionic_lif *lif, unsigned int rx_mode)
>  		lif->rx_mode = rx_mode;
>  }
>  
> -static void _ionic_lif_rx_mode(struct ionic_lif *lif, unsigned int
> rx_mode,
> -			       bool from_ndo)
> -{
> -	struct ionic_deferred_work *work;
> -
> -	if (from_ndo) {
> -		work = kzalloc(sizeof(*work), GFP_ATOMIC);
> -		if (!work) {
> -			netdev_err(lif->netdev, "%s OOM\n", __func__);
> -			return;
> -		}
> -		work->type = IONIC_DW_TYPE_RX_MODE;
> -		work->rx_mode = rx_mode;
> -		netdev_dbg(lif->netdev, "deferred: rx_mode\n");
> -		ionic_lif_deferred_enqueue(&lif->deferred, work);
> -	} else {
> -		ionic_lif_rx_mode(lif, rx_mode);
> -	}
> -}
> -
>  static void ionic_set_rx_mode(struct net_device *netdev, bool
> from_ndo)
>  {
>  	struct ionic_lif *lif = netdev_priv(netdev);
> +	struct ionic_deferred_work *work;
>  	unsigned int nfilters;
>  	unsigned int rx_mode;
>  
> @@ -1197,8 +1178,21 @@ static void ionic_set_rx_mode(struct
> net_device *netdev, bool from_ndo)
>  			rx_mode &= ~IONIC_RX_MODE_F_ALLMULTI;
>  	}
>  
> -	if (lif->rx_mode != rx_mode)
> -		_ionic_lif_rx_mode(lif, rx_mode, from_ndo);
> +	if (lif->rx_mode != rx_mode) {
> +		if (from_ndo) {
> +			work = kzalloc(sizeof(*work), GFP_ATOMIC);
> +			if (!work) {
> +				netdev_err(lif->netdev, "%s OOM\n",
> __func__);
> +				return;
> +			}
> +			work->type = IONIC_DW_TYPE_RX_MODE;
> +			work->rx_mode = rx_mode;
> +			netdev_dbg(lif->netdev, "deferred: rx_mode\n");
> +			ionic_lif_deferred_enqueue(&lif->deferred,
> work);
> +		} else {
> +			ionic_lif_rx_mode(lif, rx_mode);
> +		}
> +	}
>  }

You could move this logic one level up and totally eliminate the if
condition 

ionic_set_rx_mode_needed() {
      //sync driver data base
      return lif->rx_mode != rx_mode;
}

ndo_set_rx_mode() {
      if (!ionic_set_rx_mode_needed())
            return; // no change;
      schedule_work(set_rx_mode_hw);
}

none_ndo_set_rx_mode() {
      if (!ionic_set_rx_mode_needed())
            return; // no change;
      set_rx_mode_hw();
}

Future improvement:

One more thing I've noticed about you current ionic_set_rx_mode()
is that in case of from_ndo, when it syncs mac addresses it will
schedule a deferred mac address update work to hw per address. which i
think is an overkill, a simpler design which will totally eliminate the
need for from_ndo flags, is to do similar to the above but with a minor
change.

ionic_set_rx_mode_needed() {
      // Just sync driver mac table here and update hw later
      // in one deferred work rather than scheduling multi work
      addr_changed = ionic_dev_uc_sync();
      addr_changed |= ionic_dev_mc_sync();
      rx_mode_changed = sync_driver_rx_mode(rx_mode);

      return rx_mode_changed || addr_changed;
}

/* might sleep */
set_rx_mode_hw() {
      commit_addr_change_to_hw();
      commit_rx_mode_changes_to_hw();
}

ndo_set_rx_mode() {
      if (!ionic_set_rx_mode_needed())
            return; // no change;
      schedule_work(set_rx_mode_hw);
}

none_ndo_set_rx_mode() {
      if (!ionic_set_rx_mode_needed())
            return; // no change;
      set_rx_mode_hw();
}
Shannon Nelson Nov. 7, 2020, 1:17 a.m. UTC | #4
On 11/6/20 1:33 PM, Saeed Mahameed wrote:
> On Thu, 2020-11-05 at 16:12 -0800, Shannon Nelson wrote:
>> The _ionic_lif_rx_mode() is only used once and really doesn't
>> need to be broken out.
>>
>> Signed-off-by: Shannon Nelson <snelson@pensando.io>
>> ---
>>   .../net/ethernet/pensando/ionic/ionic_lif.c   | 38 ++++++++---------
>> --
>>   1 file changed, 16 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> index a0d26fe4cbc3..ef092ee33e59 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> @@ -1129,29 +1129,10 @@ static void ionic_lif_rx_mode(struct
>> ionic_lif *lif, unsigned int rx_mode)
>>   		lif->rx_mode = rx_mode;
>>   }
>>   
>> -static void _ionic_lif_rx_mode(struct ionic_lif *lif, unsigned int
>> rx_mode,
>> -			       bool from_ndo)
>> -{
>> -	struct ionic_deferred_work *work;
>> -
>> -	if (from_ndo) {
>> -		work = kzalloc(sizeof(*work), GFP_ATOMIC);
>> -		if (!work) {
>> -			netdev_err(lif->netdev, "%s OOM\n", __func__);
>> -			return;
>> -		}
>> -		work->type = IONIC_DW_TYPE_RX_MODE;
>> -		work->rx_mode = rx_mode;
>> -		netdev_dbg(lif->netdev, "deferred: rx_mode\n");
>> -		ionic_lif_deferred_enqueue(&lif->deferred, work);
>> -	} else {
>> -		ionic_lif_rx_mode(lif, rx_mode);
>> -	}
>> -}
>> -
>>   static void ionic_set_rx_mode(struct net_device *netdev, bool
>> from_ndo)
>>   {
>>   	struct ionic_lif *lif = netdev_priv(netdev);
>> +	struct ionic_deferred_work *work;
>>   	unsigned int nfilters;
>>   	unsigned int rx_mode;
>>   
>> @@ -1197,8 +1178,21 @@ static void ionic_set_rx_mode(struct
>> net_device *netdev, bool from_ndo)
>>   			rx_mode &= ~IONIC_RX_MODE_F_ALLMULTI;
>>   	}
>>   
>> -	if (lif->rx_mode != rx_mode)
>> -		_ionic_lif_rx_mode(lif, rx_mode, from_ndo);
>> +	if (lif->rx_mode != rx_mode) {
>> +		if (from_ndo) {
>> +			work = kzalloc(sizeof(*work), GFP_ATOMIC);
>> +			if (!work) {
>> +				netdev_err(lif->netdev, "%s OOM\n",
>> __func__);
>> +				return;
>> +			}
>> +			work->type = IONIC_DW_TYPE_RX_MODE;
>> +			work->rx_mode = rx_mode;
>> +			netdev_dbg(lif->netdev, "deferred: rx_mode\n");
>> +			ionic_lif_deferred_enqueue(&lif->deferred,
>> work);
>> +		} else {
>> +			ionic_lif_rx_mode(lif, rx_mode);
>> +		}
>> +	}
>>   }
> You could move this logic one level up and totally eliminate the if
> condition
>
> ionic_set_rx_mode_needed() {
>        //sync driver data base
>        return lif->rx_mode != rx_mode;
> }
>
> ndo_set_rx_mode() {
>        if (!ionic_set_rx_mode_needed())
>              return; // no change;
>        schedule_work(set_rx_mode_hw);
> }
>
> none_ndo_set_rx_mode() {
>        if (!ionic_set_rx_mode_needed())
>              return; // no change;
>        set_rx_mode_hw();
> }

Hmm... yes, that's possible, but I like keeping that bit of logic 
together with the rest in the main set_rx_mode block.

> Future improvement:
>
> One more thing I've noticed about you current ionic_set_rx_mode()
> is that in case of from_ndo, when it syncs mac addresses it will
> schedule a deferred mac address update work to hw per address. which i
> think is an overkill,

This is much less of an issue with the recent change in 
ionic_lif_deferred_work() to run through the whole work list in one 
deferred_work session.

sln

> a simpler design which will totally eliminate the
> need for from_ndo flags, is to do similar to the above but with a minor
> change.
>
> ionic_set_rx_mode_needed() {
>        // Just sync driver mac table here and update hw later
>        // in one deferred work rather than scheduling multi work
>        addr_changed = ionic_dev_uc_sync();
>        addr_changed |= ionic_dev_mc_sync();
>        rx_mode_changed = sync_driver_rx_mode(rx_mode);
>
>        return rx_mode_changed || addr_changed;
> }
>
> /* might sleep */
> set_rx_mode_hw() {
>        commit_addr_change_to_hw();
>        commit_rx_mode_changes_to_hw();
> }
>
> ndo_set_rx_mode() {
>        if (!ionic_set_rx_mode_needed())
>              return; // no change;
>        schedule_work(set_rx_mode_hw);
> }
>
> none_ndo_set_rx_mode() {
>        if (!ionic_set_rx_mode_needed())
>              return; // no change;
>        set_rx_mode_hw();
> }
>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
index a0d26fe4cbc3..ef092ee33e59 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
@@ -1129,29 +1129,10 @@  static void ionic_lif_rx_mode(struct ionic_lif *lif, unsigned int rx_mode)
 		lif->rx_mode = rx_mode;
 }
 
-static void _ionic_lif_rx_mode(struct ionic_lif *lif, unsigned int rx_mode,
-			       bool from_ndo)
-{
-	struct ionic_deferred_work *work;
-
-	if (from_ndo) {
-		work = kzalloc(sizeof(*work), GFP_ATOMIC);
-		if (!work) {
-			netdev_err(lif->netdev, "%s OOM\n", __func__);
-			return;
-		}
-		work->type = IONIC_DW_TYPE_RX_MODE;
-		work->rx_mode = rx_mode;
-		netdev_dbg(lif->netdev, "deferred: rx_mode\n");
-		ionic_lif_deferred_enqueue(&lif->deferred, work);
-	} else {
-		ionic_lif_rx_mode(lif, rx_mode);
-	}
-}
-
 static void ionic_set_rx_mode(struct net_device *netdev, bool from_ndo)
 {
 	struct ionic_lif *lif = netdev_priv(netdev);
+	struct ionic_deferred_work *work;
 	unsigned int nfilters;
 	unsigned int rx_mode;
 
@@ -1197,8 +1178,21 @@  static void ionic_set_rx_mode(struct net_device *netdev, bool from_ndo)
 			rx_mode &= ~IONIC_RX_MODE_F_ALLMULTI;
 	}
 
-	if (lif->rx_mode != rx_mode)
-		_ionic_lif_rx_mode(lif, rx_mode, from_ndo);
+	if (lif->rx_mode != rx_mode) {
+		if (from_ndo) {
+			work = kzalloc(sizeof(*work), GFP_ATOMIC);
+			if (!work) {
+				netdev_err(lif->netdev, "%s OOM\n", __func__);
+				return;
+			}
+			work->type = IONIC_DW_TYPE_RX_MODE;
+			work->rx_mode = rx_mode;
+			netdev_dbg(lif->netdev, "deferred: rx_mode\n");
+			ionic_lif_deferred_enqueue(&lif->deferred, work);
+		} else {
+			ionic_lif_rx_mode(lif, rx_mode);
+		}
+	}
 }
 
 static void ionic_ndo_set_rx_mode(struct net_device *netdev)