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