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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <9f334d1e-491f-565e-02fc-1a769e4f1b79@redhat.com>
Date:   Mon, 4 Mar 2019 14:09:30 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Fenghua Yu <fenghua.yu@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        H Peter Anvin <hpa@...or.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Ashok Raj <ashok.raj@...el.com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        Xiaoyao Li <xiaoyao.li@...el.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        x86 <x86@...nel.org>, kvm@...r.kernel.org
Subject: Re: [PATCH v4 03/17] wlcore: Align reg_ch_conf_pending and
 tmp_ch_bitmap to unsigned long for better performance

On 04/03/19 13:41, Peter Zijlstra wrote:
> On Mon, Mar 04, 2019 at 11:46:52AM +0100, Paolo Bonzini wrote:
>> From: Paolo Bonzini <pbonzini@...hat.com>
>> Subject: [PATCH] wlcore: simplify/fix/optimize reg_ch_conf_pending operations
>>
>> Bitmaps are defined on unsigned longs, so the usage of u32[2] in the
>> wlcore driver is incorrect.  As noted by Peter Zijlstra, casting arrays
>> to a bitmap is incorrect for big-endian architectures.
>>
>> When looking at it I observed that:
>>
>> - operations on reg_ch_conf_pending is always under the wl_lock mutex,
>> so set_bit is overkill
>>
>> - the only case where reg_ch_conf_pending is accessed a u32 at a time is
>> unnecessary too.
>>
>> This patch cleans up everything in this area, and changes tmp_ch_bitmap
>> to have the proper alignment.
>>
>> Reported-by: Fenghua Yu <fenghua.yu@...el.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
>>
>> diff --git a/drivers/net/wireless/ti/wlcore/cmd.c b/drivers/net/wireless/ti/wlcore/cmd.c
>> index 903968735a74..3e093f3a7ec8 100644
>> --- a/drivers/net/wireless/ti/wlcore/cmd.c
>> +++ b/drivers/net/wireless/ti/wlcore/cmd.c
>> @@ -1700,14 +1700,14 @@ void wlcore_set_pending_regdomain_ch(struct wl1271 *wl, u16 channel,
>>  	ch_bit_idx = wlcore_get_reg_conf_ch_idx(band, channel);
>>  
>>  	if (ch_bit_idx >= 0 && ch_bit_idx <= WL1271_MAX_CHANNELS)
>> -		set_bit(ch_bit_idx, (long *)wl->reg_ch_conf_pending);
>> +		__set_bit_le(ch_bit_idx, (long *)wl->reg_ch_conf_pending);
>>  }
>>  
>>  int wlcore_cmd_regdomain_config_locked(struct wl1271 *wl)
>>  {
>>  	struct wl12xx_cmd_regdomain_dfs_config *cmd = NULL;
>>  	int ret = 0, i, b, ch_bit_idx;
>> -	u32 tmp_ch_bitmap[2];
>> +	u32 tmp_ch_bitmap[2] __aligned(sizeof(unsigned long));
> 
> Also mark it as __le32 ?

That would require more changes to mark ch_bit_map1/ch_bit_map2 as
__le32 (I think, I don't do much sparse), so I didn't do that.

>>  	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));
> 
> How about using:
> 
> 	bitmap_to_arr32(tmp_ch_bitmap, wl->reg_ch_conf_pending, sizeof(tmp_ch_bitmap));
> 	for (i=0; i<2; i++)
> 		tmp_ch_bitmap[i] = cpu_to_le32(tmp_ch_bitmap[i]);
> 
> (or add bitmap_to_arr32_le ?)

I've used __set_bit_le when setting reg_ch_conf_pending so no need to
swizzle here; OTOH bitmap_to_arr32 doesn't work here that swizzle
already swaps halfwords.

>>  	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);
> 
> But you copied in reg_ch_conf_pending without doing an LE swizzle.
> With the proposed change, we have two __le32 here and it works again.

(Again there's no need to do an LE swizzle because it's done in
wlcore_set_pending_regdomain_ch).

>>  		}
>>  	}
>>  
>> -	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;
>>  
> 
> And then remove the cpu_to_le32() on assignment to ch_bit_map*.

Yup, I forgot to commit that cpu_to_le32 removal.

>> 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);
> 
> Is never actually used as a bitmap but used as opaque storage with
> memcpy and memcmp against tmp_ch_bitmap.

Yeah, but it is the easiest way to ensure it is the right size as
reg_ch_conf_pending.  The two are related, it makes sense to declare
them the same.

Paolo