Re: [PATCH v4 03/17] wlcore: Align reg_ch_conf_pending and tmp_ch_bitmap to unsigned long for better performance

From: Paolo Bonzini <pbonzini@redhat.com>
To: Fenghua Yu <fenghua.yu@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	H Peter Anvin <hpa@zytor.com>,
	Dave Hansen <dave.hansen@intel.com>,
	Ashok Raj <ashok.raj@intel.com>,
	Ravi V Shankar <ravi.v.shankar@intel.com>,
	Xiaoyao Li <xiaoyao.li@intel.com>,
	linux-kernel <linux-kernel@vger.kernel.org>, x86 <x86@kernel.org>,
	kvm@vger.kernel.org
Subject: Re: [PATCH v4 03/17] wlcore: Align reg_ch_conf_pending and tmp_ch_bitmap to unsigned long for better performance
Date: Mon, 4 Mar 2019 16:54:23 +0100	[thread overview]
Message-ID: <7312a322-aae1-3aab-c8af-e16a5d8ee316@redhat.com> (raw)
In-Reply-To: <20190304144050.GA230561@romley-ivt3.sc.intel.com>

On 04/03/19 15:40, Fenghua Yu wrote:

>> +	u32 tmp_ch_bitmap[2] __aligned(sizeof(unsigned long));
> 
> Now __aligned() is unnecessary because __set_bit_le() handles tmp_ch_bitmap,
> right?

It's still needed because, as explained by Peter, bitmap functions
should always operate on properly aligned variables.

>>  	struct wiphy *wiphy = wl->hw->wiphy;
>>  	struct ieee80211_supported_band *band;
>>  	bool timeout = false;
>> @@ -1717,7 +1717,7 @@ int wlcore_cmd_regdomain_config_locked(struct wl1271 *wl)
>>  
>>  	wl1271_debug(DEBUG_CMD, "cmd reg domain config");
>>  
>> -	memset(tmp_ch_bitmap, 0, sizeof(tmp_ch_bitmap));
>> +	memcpy(tmp_ch_bitmap, wl->reg_ch_conf_pending, sizeof(tmp_ch_bitmap));
>>  
>>  	for (b = NL80211_BAND_2GHZ; b <= NL80211_BAND_5GHZ; b++) {
>>  		band = wiphy->bands[b];
>> @@ -1738,13 +1738,10 @@ int wlcore_cmd_regdomain_config_locked(struct wl1271 *wl)
>>  			if (ch_bit_idx < 0)
>>  				continue;
>>  
>> -			set_bit(ch_bit_idx, (long *)tmp_ch_bitmap);
>> +			__set_bit_le(ch_bit_idx, (long *)tmp_ch_bitmap);
> 
> Is __test_and_set_bit_le() more meaningful to avoid duplicate bit setting ?

No, since the return value would be unused.

Note that this patch is missing the removal of cpu_to_le32, as noticed
by Peter.

Thanks,

Paolo

> 
>>  		}
>>  	}
>>  
>> -	tmp_ch_bitmap[0] |= wl->reg_ch_conf_pending[0];
>> -	tmp_ch_bitmap[1] |= wl->reg_ch_conf_pending[1];
>> -
>>  	if (!memcmp(tmp_ch_bitmap, wl->reg_ch_conf_last, sizeof(tmp_ch_bitmap)))
>>  		goto out;
>>  
>> diff --git a/drivers/net/wireless/ti/wlcore/wlcore.h b/drivers/net/wireless/ti/wlcore/wlcore.h
>> index dd14850b0603..870eea3e7a27 100644
>> --- a/drivers/net/wireless/ti/wlcore/wlcore.h
>> +++ b/drivers/net/wireless/ti/wlcore/wlcore.h
>> @@ -320,9 +320,9 @@ struct wl1271 {
>>  	bool watchdog_recovery;
>>  
>>  	/* Reg domain last configuration */
>> -	u32 reg_ch_conf_last[2]  __aligned(8);
>> +	DECLARE_BITMAP(reg_ch_conf_last, 64);
>>  	/* Reg domain pending configuration */
>> -	u32 reg_ch_conf_pending[2];
>> +	DECLARE_BITMAP(reg_ch_conf_pending, 64);
>>  
>>  	/* Pointer that holds DMA-friendly block for the mailbox */
>>  	void *mbox;