| Message ID | cover.1661407821.git.alibuda@linux.alibaba.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | optimize the parallelism of SMC-R connections | expand |
On Fri, 26 Aug 2022 17:51:27 +0800 D. Wythe wrote: > This patch set attempts to optimize the parallelism of SMC-R connections, > mainly to reduce unnecessary blocking on locks, and to fix exceptions that > occur after thoses optimization. > > According to Off-CPU graph, SMC worker's off-CPU as that: > > smc_close_passive_work (1.09%) > smcr_buf_unuse (1.08%) > smc_llc_flow_initiate (1.02%) > > smc_listen_work (48.17%) > __mutex_lock.isra.11 (47.96%) The patches should be ordered so that the prerequisite changes are first, then the removal of locks. Looks like there are 3 patches here which carry a Fixes tag, for an old commit but in fact IIUC there is no bug in those old commits, the problem only appears after the locking is removed? That said please wait for IBM folks to review first before reshuffling the patches, I presume the code itself won't change. Also I still haven't see anyone reply to Al Viro, IIRC he was complaining about changes someone from your team has made. I consider this a blocker for applying new patches from your team :(
On Fri, Aug 26, 2022 at 06:32:13PM -0700, Jakub Kicinski wrote: > On Fri, 26 Aug 2022 17:51:27 +0800 D. Wythe wrote: > > This patch set attempts to optimize the parallelism of SMC-R connections, > > mainly to reduce unnecessary blocking on locks, and to fix exceptions that > > occur after thoses optimization. > > > > According to Off-CPU graph, SMC worker's off-CPU as that: > > > > smc_close_passive_work (1.09%) > > smcr_buf_unuse (1.08%) > > smc_llc_flow_initiate (1.02%) > > > > smc_listen_work (48.17%) > > __mutex_lock.isra.11 (47.96%) > > The patches should be ordered so that the prerequisite changes are > first, then the removal of locks. Looks like there are 3 patches here > which carry a Fixes tag, for an old commit but in fact IIUC there is no > bug in those old commits, the problem only appears after the locking is > removed? > > That said please wait for IBM folks to review first before reshuffling > the patches, I presume the code itself won't change. > > Also I still haven't see anyone reply to Al Viro, IIRC he was > complaining about changes someone from your team has made. > I consider this a blocker for applying new patches from your team :( Yes, the approach of replacing socket needs to be refactored, and I have been working on it for the fixes. Maybe I missed something, you can check this reply here [1]. [1] https://lore.kernel.org/all/YvTL%2Fsf6lrhuGDuy@TonyMac-Alibaba/ Thanks. Tony Lu
On 8/27/22 9:32 AM, Jakub Kicinski wrote: > On Fri, 26 Aug 2022 17:51:27 +0800 D. Wythe wrote: >> This patch set attempts to optimize the parallelism of SMC-R connections, >> mainly to reduce unnecessary blocking on locks, and to fix exceptions that >> occur after thoses optimization. >> >> According to Off-CPU graph, SMC worker's off-CPU as that: >> >> smc_close_passive_work (1.09%) >> smcr_buf_unuse (1.08%) >> smc_llc_flow_initiate (1.02%) >> >> smc_listen_work (48.17%) >> __mutex_lock.isra.11 (47.96%) > > The patches should be ordered so that the prerequisite changes are > first, then the removal of locks. Looks like there are 3 patches here > which carry a Fixes tag, for an old commit but in fact IIUC there is no > bug in those old commits, the problem only appears after the locking is > removed? > Thank you for your suggestion, this is indeed my ill-consideration. The first PATCH with the Fix tag is indeed a prerequisite for removing the lock, and it do should be placed before. The other two with PATCH fixes theoretically can also appear before, but after the lock is removed the probability of it will be greatly increased. I see it can also be placed before. > That said please wait for IBM folks to review first before reshuffling > the patches, I presume the code itself won't change. Thanks your suggestion again, I will reshuffling the order of it after you have reviewed it all. > Also I still haven't see anyone reply to Al Viro, IIRC he was > complaining about changes someone from your team has made. > I consider this a blocker for applying new patches from your team :( Sorry to bother you and your team, my colleague will explain to you soon. Thanks. D. Wythe
On 26.08.2022 11:51, D. Wythe wrote: > From: "D. Wythe" <alibuda@linux.alibaba.com> > > This patch set attempts to optimize the parallelism of SMC-R connections, > mainly to reduce unnecessary blocking on locks, and to fix exceptions that > occur after thoses optimization. > > According to Off-CPU graph, SMC worker's off-CPU as that: > > smc_close_passive_work (1.09%) > smcr_buf_unuse (1.08%) > smc_llc_flow_initiate (1.02%) > > smc_listen_work (48.17%) > __mutex_lock.isra.11 (47.96%) > > > An ideal SMC-R connection process should only block on the IO events > of the network, but it's quite clear that the SMC-R connection now is > queued on the lock most of the time. > > The goal of this patchset is to achieve our ideal situation where > network IO events are blocked for the majority of the connection lifetime. > > There are three big locks here: > > 1. smc_client_lgr_pending & smc_server_lgr_pending > > 2. llc_conf_mutex > > 3. rmbs_lock & sndbufs_lock > > And an implementation issue: > > 1. confirm/delete rkey msg can't be sent concurrently while > protocol allows indeed. > > Unfortunately,The above problems together affect the parallelism of > SMC-R connection. If any of them are not solved. our goal cannot > be achieved. > > After this patch set, we can get a quite ideal off-CPU graph as > following: > > smc_close_passive_work (41.58%) > smcr_buf_unuse (41.57%) > smc_llc_do_delete_rkey (41.57%) > > smc_listen_work (39.10%) > smc_clc_wait_msg (13.18%) > tcp_recvmsg_locked (13.18) > smc_listen_find_device (25.87%) > smcr_lgr_reg_rmbs (25.87%) > smc_llc_do_confirm_rkey (25.87%) > > We can see that most of the waiting times are waiting for network IO > events. This also has a certain performance improvement on our > short-lived conenction wrk/nginx benchmark test: > > +--------------+------+------+-------+--------+------+--------+ > |conns/qps |c4 | c8 | c16 | c32 | c64 | c200 | > +--------------+------+------+-------+--------+------+--------+ > |SMC-R before |9.7k | 10k | 10k | 9.9k | 9.1k | 8.9k | > +--------------+------+------+-------+--------+------+--------+ > |SMC-R now |13k | 19k | 18k | 16k | 15k | 12k | > +--------------+------+------+-------+--------+------+--------+ > |TCP |15k | 35k | 51k | 80k | 100k | 162k | > +--------------+------+------+-------+--------+------+--------+ > > The reason why the benefit is not obvious after the number of connections > has increased dues to workqueue. If we try to change workqueue to UNBOUND, > we can obtain at least 4-5 times performance improvement, reach up to half > of TCP. However, this is not an elegant solution, the optimization of it > will be much more complicated. But in any case, we will submit relevant > optimization patches as soon as possible. > > Please note that the premise here is that the lock related problem > must be solved first, otherwise, no matter how we optimize the workqueue, > there won't be much improvement. > > Because there are a lot of related changes to the code, if you have > any questions or suggestions, please let me know. > > Thanks > D. Wythe > > v1 -> v2: > > 1. Fix panic in SMC-D scenario > 2. Fix lnkc related hashfn calculation exception, caused by operator > priority. > 3. Remove -EBUSY processing of rhashtable_insert_fast, see more details > in comments around smcr_link_get_or_create_cluster(). > 4. Only wake up one connection if the link has not been active. > 5. Delete obsolete unlock logic in smc_listen_work(). > 6. PATCH format, do Reverse Christmas tree. > 7. PATCH format, change all xxx_lnk_xxx function to xxx_link_xxx. > 8. PATCH format, add correct fix tag for the patches for fixes. > 9. PATCH format, fix some spelling error. > 10.PATCH format, rename slow to do_slow in smcr_lgr_reg_rmbs(). > > > D. Wythe (10): > net/smc: remove locks smc_client_lgr_pending and > smc_server_lgr_pending > net/smc: fix SMC_CLC_DECL_ERR_REGRMB without smc_server_lgr_pending > net/smc: allow confirm/delete rkey response deliver multiplex > net/smc: make SMC_LLC_FLOW_RKEY run concurrently > net/smc: llc_conf_mutex refactor, replace it with rw_semaphore > net/smc: use read semaphores to reduce unnecessary blocking in > smc_buf_create() & smcr_buf_unuse() > net/smc: reduce unnecessary blocking in smcr_lgr_reg_rmbs() > net/smc: replace mutex rmbs_lock and sndbufs_lock with rw_semaphore > net/smc: Fix potential panic dues to unprotected > smc_llc_srv_add_link() > net/smc: fix application data exception > > net/smc/af_smc.c | 42 +++-- > net/smc/smc_core.c | 443 +++++++++++++++++++++++++++++++++++++++++++++++------ > net/smc/smc_core.h | 78 +++++++++- > net/smc/smc_llc.c | 286 +++++++++++++++++++++++++--------- > net/smc/smc_llc.h | 6 + > net/smc/smc_wr.c | 10 -- > net/smc/smc_wr.h | 10 ++ > 7 files changed, 725 insertions(+), 150 deletions(-) > D., I'm sorry. I replied to the patch 01/10 with the test results and not the cover letter. I have a filter on my inbox separating everything for "net/smc:" and the keywords are missing on this cover letter. Mea culpa. https://lore.kernel.org/netdev/1767b6e4-0053-728b-9722-add68da13781@linux.ibm.com/ - Jan
From: "D. Wythe" <alibuda@linux.alibaba.com> This patch set attempts to optimize the parallelism of SMC-R connections, mainly to reduce unnecessary blocking on locks, and to fix exceptions that occur after thoses optimization. According to Off-CPU graph, SMC worker's off-CPU as that: smc_close_passive_work (1.09%) smcr_buf_unuse (1.08%) smc_llc_flow_initiate (1.02%) smc_listen_work (48.17%) __mutex_lock.isra.11 (47.96%) An ideal SMC-R connection process should only block on the IO events of the network, but it's quite clear that the SMC-R connection now is queued on the lock most of the time. The goal of this patchset is to achieve our ideal situation where network IO events are blocked for the majority of the connection lifetime. There are three big locks here: 1. smc_client_lgr_pending & smc_server_lgr_pending 2. llc_conf_mutex 3. rmbs_lock & sndbufs_lock And an implementation issue: 1. confirm/delete rkey msg can't be sent concurrently while protocol allows indeed. Unfortunately,The above problems together affect the parallelism of SMC-R connection. If any of them are not solved. our goal cannot be achieved. After this patch set, we can get a quite ideal off-CPU graph as following: smc_close_passive_work (41.58%) smcr_buf_unuse (41.57%) smc_llc_do_delete_rkey (41.57%) smc_listen_work (39.10%) smc_clc_wait_msg (13.18%) tcp_recvmsg_locked (13.18) smc_listen_find_device (25.87%) smcr_lgr_reg_rmbs (25.87%) smc_llc_do_confirm_rkey (25.87%) We can see that most of the waiting times are waiting for network IO events. This also has a certain performance improvement on our short-lived conenction wrk/nginx benchmark test: +--------------+------+------+-------+--------+------+--------+ |conns/qps |c4 | c8 | c16 | c32 | c64 | c200 | +--------------+------+------+-------+--------+------+--------+ |SMC-R before |9.7k | 10k | 10k | 9.9k | 9.1k | 8.9k | +--------------+------+------+-------+--------+------+--------+ |SMC-R now |13k | 19k | 18k | 16k | 15k | 12k | +--------------+------+------+-------+--------+------+--------+ |TCP |15k | 35k | 51k | 80k | 100k | 162k | +--------------+------+------+-------+--------+------+--------+ The reason why the benefit is not obvious after the number of connections has increased dues to workqueue. If we try to change workqueue to UNBOUND, we can obtain at least 4-5 times performance improvement, reach up to half of TCP. However, this is not an elegant solution, the optimization of it will be much more complicated. But in any case, we will submit relevant optimization patches as soon as possible. Please note that the premise here is that the lock related problem must be solved first, otherwise, no matter how we optimize the workqueue, there won't be much improvement. Because there are a lot of related changes to the code, if you have any questions or suggestions, please let me know. Thanks D. Wythe v1 -> v2: 1. Fix panic in SMC-D scenario 2. Fix lnkc related hashfn calculation exception, caused by operator priority. 3. Remove -EBUSY processing of rhashtable_insert_fast, see more details in comments around smcr_link_get_or_create_cluster(). 4. Only wake up one connection if the link has not been active. 5. Delete obsolete unlock logic in smc_listen_work(). 6. PATCH format, do Reverse Christmas tree. 7. PATCH format, change all xxx_lnk_xxx function to xxx_link_xxx. 8. PATCH format, add correct fix tag for the patches for fixes. 9. PATCH format, fix some spelling error. 10.PATCH format, rename slow to do_slow in smcr_lgr_reg_rmbs(). D. Wythe (10): net/smc: remove locks smc_client_lgr_pending and smc_server_lgr_pending net/smc: fix SMC_CLC_DECL_ERR_REGRMB without smc_server_lgr_pending net/smc: allow confirm/delete rkey response deliver multiplex net/smc: make SMC_LLC_FLOW_RKEY run concurrently net/smc: llc_conf_mutex refactor, replace it with rw_semaphore net/smc: use read semaphores to reduce unnecessary blocking in smc_buf_create() & smcr_buf_unuse() net/smc: reduce unnecessary blocking in smcr_lgr_reg_rmbs() net/smc: replace mutex rmbs_lock and sndbufs_lock with rw_semaphore net/smc: Fix potential panic dues to unprotected smc_llc_srv_add_link() net/smc: fix application data exception net/smc/af_smc.c | 42 +++-- net/smc/smc_core.c | 443 +++++++++++++++++++++++++++++++++++++++++++++++------ net/smc/smc_core.h | 78 +++++++++- net/smc/smc_llc.c | 286 +++++++++++++++++++++++++--------- net/smc/smc_llc.h | 6 + net/smc/smc_wr.c | 10 -- net/smc/smc_wr.h | 10 ++ 7 files changed, 725 insertions(+), 150 deletions(-)