netdev - Re: [PATCH v2 net-next 6/8] ionic: flatten calls to ionic_lif_rx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <6ebe62b9-b5ea-0241-acaa-c43def591683@pensando.io>
Date:   Fri, 6 Nov 2020 17:17:49 -0800
From:   Shannon Nelson <snelson@...sando.io>
To:     Saeed Mahameed <saeed@...nel.org>, netdev@...r.kernel.org,
        davem@...emloft.net, kuba@...nel.org
Subject: Re: [PATCH v2 net-next 6/8] ionic: flatten calls to ionic_lif_rx_mode

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@...sando.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();
> }
>