| Message ID | 20220130180256.28303-2-tonylu@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/smc: Improvements for TCP_CORK and sendfile() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 59 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On 1/30/22 19:02, Tony Lu wrote: > According to the man page of TCP_CORK [1], if set, don't send out > partial frames. All queued partial frames are sent when option is > cleared again. > > When applications call setsockopt to disable TCP_CORK, this call is > protected by lock_sock(), and tries to mod_delayed_work() to 0, in order > to send pending data right now. However, the delayed work smc_tx_work is > also protected by lock_sock(). There introduces lock contention for > sending data. > > To fix it, send pending data directly which acts like TCP, without > lock_sock() protected in the context of setsockopt (already lock_sock()ed), > and cancel unnecessary dealyed work, which is protected by lock. > > [1] https://linux.die.net/man/7/tcp > > Signed-off-by: Tony Lu <tonylu@linux.alibaba.com> > --- > net/smc/af_smc.c | 4 ++-- > net/smc/smc_tx.c | 25 +++++++++++++++---------- > net/smc/smc_tx.h | 1 + > 3 files changed, 18 insertions(+), 12 deletions(-) > > diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c > index ffab9cee747d..ef021ec6b361 100644 > --- a/net/smc/af_smc.c > +++ b/net/smc/af_smc.c > @@ -2600,8 +2600,8 @@ static int smc_setsockopt(struct socket *sock, int level, int optname, > sk->sk_state != SMC_CLOSED) { > if (!val) { > SMC_STAT_INC(smc, cork_cnt); > - mod_delayed_work(smc->conn.lgr->tx_wq, > - &smc->conn.tx_work, 0); > + smc_tx_pending(&smc->conn); > + cancel_delayed_work(&smc->conn.tx_work); > } > } > break; > diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c > index be241d53020f..7b0b6e24582f 100644 > --- a/net/smc/smc_tx.c > +++ b/net/smc/smc_tx.c > @@ -597,27 +597,32 @@ int smc_tx_sndbuf_nonempty(struct smc_connection *conn) > return rc; > } > > -/* Wakeup sndbuf consumers from process context > - * since there is more data to transmit > - */ > -void smc_tx_work(struct work_struct *work) > +void smc_tx_pending(struct smc_connection *conn) Could you add a comment that we're expecting lock_sock() to be held when calling this function? Thanks, Stefan
On Mon, Jan 31, 2022 at 08:13:53PM +0100, Stefan Raspl wrote: > On 1/30/22 19:02, Tony Lu wrote: > > According to the man page of TCP_CORK [1], if set, don't send out > > partial frames. All queued partial frames are sent when option is > > cleared again. > > > > When applications call setsockopt to disable TCP_CORK, this call is > > protected by lock_sock(), and tries to mod_delayed_work() to 0, in order > > to send pending data right now. However, the delayed work smc_tx_work is > > also protected by lock_sock(). There introduces lock contention for > > sending data. > > > > To fix it, send pending data directly which acts like TCP, without > > lock_sock() protected in the context of setsockopt (already lock_sock()ed), > > and cancel unnecessary dealyed work, which is protected by lock. > > > > [1] https://linux.die.net/man/7/tcp > > > > Signed-off-by: Tony Lu <tonylu@linux.alibaba.com> > > --- > > net/smc/af_smc.c | 4 ++-- > > net/smc/smc_tx.c | 25 +++++++++++++++---------- > > net/smc/smc_tx.h | 1 + > > 3 files changed, 18 insertions(+), 12 deletions(-) > > > > diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c > > index ffab9cee747d..ef021ec6b361 100644 > > --- a/net/smc/af_smc.c > > +++ b/net/smc/af_smc.c > > @@ -2600,8 +2600,8 @@ static int smc_setsockopt(struct socket *sock, int level, int optname, > > sk->sk_state != SMC_CLOSED) { > > if (!val) { > > SMC_STAT_INC(smc, cork_cnt); > > - mod_delayed_work(smc->conn.lgr->tx_wq, > > - &smc->conn.tx_work, 0); > > + smc_tx_pending(&smc->conn); > > + cancel_delayed_work(&smc->conn.tx_work); > > } > > } > > break; > > diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c > > index be241d53020f..7b0b6e24582f 100644 > > --- a/net/smc/smc_tx.c > > +++ b/net/smc/smc_tx.c > > @@ -597,27 +597,32 @@ int smc_tx_sndbuf_nonempty(struct smc_connection *conn) > > return rc; > > } > > -/* Wakeup sndbuf consumers from process context > > - * since there is more data to transmit > > - */ > > -void smc_tx_work(struct work_struct *work) > > +void smc_tx_pending(struct smc_connection *conn) > > Could you add a comment that we're expecting lock_sock() to be held when > calling this function? I will add it in the next separated patch. Thank you, Tony Lu
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c index ffab9cee747d..ef021ec6b361 100644 --- a/net/smc/af_smc.c +++ b/net/smc/af_smc.c @@ -2600,8 +2600,8 @@ static int smc_setsockopt(struct socket *sock, int level, int optname, sk->sk_state != SMC_CLOSED) { if (!val) { SMC_STAT_INC(smc, cork_cnt); - mod_delayed_work(smc->conn.lgr->tx_wq, - &smc->conn.tx_work, 0); + smc_tx_pending(&smc->conn); + cancel_delayed_work(&smc->conn.tx_work); } } break; diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c index be241d53020f..7b0b6e24582f 100644 --- a/net/smc/smc_tx.c +++ b/net/smc/smc_tx.c @@ -597,27 +597,32 @@ int smc_tx_sndbuf_nonempty(struct smc_connection *conn) return rc; } -/* Wakeup sndbuf consumers from process context - * since there is more data to transmit - */ -void smc_tx_work(struct work_struct *work) +void smc_tx_pending(struct smc_connection *conn) { - struct smc_connection *conn = container_of(to_delayed_work(work), - struct smc_connection, - tx_work); struct smc_sock *smc = container_of(conn, struct smc_sock, conn); int rc; - lock_sock(&smc->sk); if (smc->sk.sk_err) - goto out; + return; rc = smc_tx_sndbuf_nonempty(conn); if (!rc && conn->local_rx_ctrl.prod_flags.write_blocked && !atomic_read(&conn->bytes_to_rcv)) conn->local_rx_ctrl.prod_flags.write_blocked = 0; +} + +/* Wakeup sndbuf consumers from process context + * since there is more data to transmit + */ +void smc_tx_work(struct work_struct *work) +{ + struct smc_connection *conn = container_of(to_delayed_work(work), + struct smc_connection, + tx_work); + struct smc_sock *smc = container_of(conn, struct smc_sock, conn); -out: + lock_sock(&smc->sk); + smc_tx_pending(conn); release_sock(&smc->sk); } diff --git a/net/smc/smc_tx.h b/net/smc/smc_tx.h index 07e6ad76224a..a59f370b8b43 100644 --- a/net/smc/smc_tx.h +++ b/net/smc/smc_tx.h @@ -27,6 +27,7 @@ static inline int smc_tx_prepared_sends(struct smc_connection *conn) return smc_curs_diff(conn->sndbuf_desc->len, &sent, &prep); } +void smc_tx_pending(struct smc_connection *conn); void smc_tx_work(struct work_struct *work); void smc_tx_init(struct smc_sock *smc); int smc_tx_sendmsg(struct smc_sock *smc, struct msghdr *msg, size_t len);
According to the man page of TCP_CORK [1], if set, don't send out partial frames. All queued partial frames are sent when option is cleared again. When applications call setsockopt to disable TCP_CORK, this call is protected by lock_sock(), and tries to mod_delayed_work() to 0, in order to send pending data right now. However, the delayed work smc_tx_work is also protected by lock_sock(). There introduces lock contention for sending data. To fix it, send pending data directly which acts like TCP, without lock_sock() protected in the context of setsockopt (already lock_sock()ed), and cancel unnecessary dealyed work, which is protected by lock. [1] https://linux.die.net/man/7/tcp Signed-off-by: Tony Lu <tonylu@linux.alibaba.com> --- net/smc/af_smc.c | 4 ++-- net/smc/smc_tx.c | 25 +++++++++++++++---------- net/smc/smc_tx.h | 1 + 3 files changed, 18 insertions(+), 12 deletions(-)