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

[Fenghua Yu]

On Mon, Mar 04, 2019 at 11:46:52AM +0100, Paolo Bonzini wrote:
> On 04/03/19 11:11, Peter Zijlstra wrote:
> > On Fri, Mar 01, 2019 at 06:44:57PM -0800, Fenghua Yu wrote:
> >> A bit in reg_ch_conf_pending in wl271 and tmp_ch_bitmap is set 
> >> atomically by set_bit(). set_bit() sets the bit in a single 
> >> unsigned long location. If the variables are not aligned to 
> >> unsigned long, set_bit() accesses two cache lines and thus causes 
> >> slower performance. On x86, this scenario is called split lock and 
> >> can cause overall performance degradation due to locked BTSL 
> >> instruction in set_bit() locks bus.
> >> 
> >> To avoid performance degradation, the two variables are aligned to
> >>  unsigned long.
> >> 
> >> Signed-off-by: Fenghua Yu <fenghua.yu@xxxxxxxxx> --- 
> >> drivers/net/wireless/ti/wlcore/cmd.c    | 3 ++- 
> >> drivers/net/wireless/ti/wlcore/wlcore.h | 6 ++++-- 2 files
> >> changed, 6 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/drivers/net/wireless/ti/wlcore/cmd.c 
> >> b/drivers/net/wireless/ti/wlcore/cmd.c index 
> >> 903968735a74..8d15a6307d44 100644 --- 
> >> a/drivers/net/wireless/ti/wlcore/cmd.c +++ 
> >> b/drivers/net/wireless/ti/wlcore/cmd.c @@ -1707,7 +1707,8 @@ 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]; +	/* Align to unsigned long
> >> for better performance in set_bit() */ +	u32 tmp_ch_bitmap[2] 
> >> __aligned(sizeof(unsigned long));
> 
> This is the only place where an array of u32 is needed, because of
> 
>         cmd->ch_bit_map1 = cpu_to_le32(tmp_ch_bitmap[0]);
>         cmd->ch_bit_map2 = cpu_to_le32(tmp_ch_bitmap[1]);
> 
> All the others should use DECLARE_BITMAP, including reg_ch_conf_last 
> which was already using __aligned.  As Peter mentioned they should
> also use set_bit_le.  Actually they do not need locked access at all
> because the only code paths to the set_bit take a mutex.
> 
> There is one other place that is accessing the items of the array, but
> it is fixed easily.  The following patch should do everything:
> 
> ------------------- 8< --------------------------
> From: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> 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@xxxxxxxxx>
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> 
> 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));

Now __aligned() is unnecessary because __set_bit_le() handles tmp_ch_bitmap,
right?

>  	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 ?

>  		}
>  	}
>  
> -	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;