| Message ID | 20200619115710.2571-1-fw@strlen.de |
|---|---|
| State | Accepted, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [mptcp-next] mptcp: use mptcp worker for path management | expand |
On Fri, 2020-06-19 at 13:57 +0200, Florian Westphal wrote: > @@ -181,35 +179,6 @@ int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc) > return mptcp_pm_nl_get_local_id(msk, skc); > } > > -static void pm_worker(struct work_struct *work) > -{ > - struct mptcp_pm_data *pm = container_of(work, struct mptcp_pm_data, > - work); > - struct mptcp_sock *msk = container_of(pm, struct mptcp_sock, pm); > - struct sock *sk = (struct sock *)msk; > - > - lock_sock(sk); > - spin_lock_bh(&msk->pm.lock); > - > - pr_debug("msk=%p status=%x", msk, pm->status); > - if (pm->status & BIT(MPTCP_PM_ADD_ADDR_RECEIVED)) { > - pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_RECEIVED); > - mptcp_pm_nl_add_addr_received(msk); > - } > - if (pm->status & BIT(MPTCP_PM_ESTABLISHED)) { > - pm->status &= ~BIT(MPTCP_PM_ESTABLISHED); > - mptcp_pm_nl_fully_established(msk); > - } > - if (pm->status & BIT(MPTCP_PM_SUBFLOW_ESTABLISHED)) { > - pm->status &= ~BIT(MPTCP_PM_SUBFLOW_ESTABLISHED); > - mptcp_pm_nl_subflow_established(msk); > - } > - > - spin_unlock_bh(&msk->pm.lock); > - release_sock(sk); > - sock_put(sk); > -} > - > void mptcp_pm_data_init(struct mptcp_sock *msk) > { > msk->pm.add_addr_signaled = 0; > @@ -223,22 +192,11 @@ void mptcp_pm_data_init(struct mptcp_sock *msk) > msk->pm.status = 0; > > spin_lock_init(&msk->pm.lock); > - INIT_WORK(&msk->pm.work, pm_worker); > > mptcp_pm_nl_data_init(msk); > } > > -void mptcp_pm_close(struct mptcp_sock *msk) > -{ > - if (cancel_work_sync(&msk->pm.work)) > - sock_put((struct sock *)msk); > -} > - > void __init mptcp_pm_init(void) > { > - pm_wq = alloc_workqueue("pm_wq", WQ_UNBOUND | WQ_MEM_RECLAIM, 8); > - if (!pm_wq) > - panic("Failed to allocate workqueue"); > - I'm wondering if we should keep using an MPTCP-specific workqueue. Server side, we don't expect the work to trigger very often, but should that ever happen, top will tell easily. WDYT? Thanks! Paolo
Paolo Abeni <pabeni@redhat.com> wrote: > I'm wondering if we should keep using an MPTCP-specific workqueue. > Server side, we don't expect the work to trigger very often, but should > that ever happen, top will tell easily. You mean switch msk->work from system_wq to a custom workqueue?
On Mon, 2020-06-22 at 13:18 +0200, Florian Westphal wrote: > Paolo Abeni <pabeni@redhat.com> wrote: > > I'm wondering if we should keep using an MPTCP-specific workqueue. > > Server side, we don't expect the work to trigger very often, but should > > that ever happen, top will tell easily. > > You mean switch msk->work from system_wq to a custom workqueue? yep, e.g. reusing and renaming 'pm_wq'. When the PM works a lot (in "ndiffports" mode as in issues/33), the pm_wq is visible in top - at least in dbg build, I never tested with regular kconfig - and I like being able to identify it easily. WDYT? Thanks! Paolo
Paolo Abeni <pabeni@redhat.com> wrote: > On Mon, 2020-06-22 at 13:18 +0200, Florian Westphal wrote: > > Paolo Abeni <pabeni@redhat.com> wrote: > > > I'm wondering if we should keep using an MPTCP-specific workqueue. > > > Server side, we don't expect the work to trigger very often, but should > > > that ever happen, top will tell easily. > > > > You mean switch msk->work from system_wq to a custom workqueue? > > yep, e.g. reusing and renaming 'pm_wq'. When the PM works a lot (in > "ndiffports" mode as in issues/33), the pm_wq is visible in top - at > least in dbg build, I never tested with regular kconfig - and I like > being able to identify it easily. WDYT? Can you do this? I am not at all sure how you would like this to look like 8-( (Merge parts of protocol.c into pm.c?)
On Mon, 2020-06-22 at 15:03 +0200, Florian Westphal wrote: > Paolo Abeni <pabeni@redhat.com> wrote: > > On Mon, 2020-06-22 at 13:18 +0200, Florian Westphal wrote: > > > Paolo Abeni <pabeni@redhat.com> wrote: > > > > I'm wondering if we should keep using an MPTCP-specific workqueue. > > > > Server side, we don't expect the work to trigger very often, but should > > > > that ever happen, top will tell easily. > > > > > > You mean switch msk->work from system_wq to a custom workqueue? > > > > yep, e.g. reusing and renaming 'pm_wq'. When the PM works a lot (in > > "ndiffports" mode as in issues/33), the pm_wq is visible in top - at > > least in dbg build, I never tested with regular kconfig - and I like > > being able to identify it easily. WDYT? > > Can you do this? I'll try to run the perf tests on non-debug build, but I see your point. Let's keep the patch simple - as is now. We can look for a workqueue only if mptcp workqueue usage proves to be relevant in non debug build. Thanks, Paolo
Hi Florian, Paolo On 22/06/2020 16:51, Paolo Abeni wrote: > On Mon, 2020-06-22 at 15:03 +0200, Florian Westphal wrote: >> Paolo Abeni <pabeni@redhat.com> wrote: >>> On Mon, 2020-06-22 at 13:18 +0200, Florian Westphal wrote: >>>> Paolo Abeni <pabeni@redhat.com> wrote: >>>>> I'm wondering if we should keep using an MPTCP-specific workqueue. >>>>> Server side, we don't expect the work to trigger very often, but should >>>>> that ever happen, top will tell easily. >>>> >>>> You mean switch msk->work from system_wq to a custom workqueue? >>> >>> yep, e.g. reusing and renaming 'pm_wq'. When the PM works a lot (in >>> "ndiffports" mode as in issues/33), the pm_wq is visible in top - at >>> least in dbg build, I never tested with regular kconfig - and I like >>> being able to identify it easily. WDYT? >> >> Can you do this? > > I'll try to run the perf tests on non-debug build, but I see your > point. Let's keep the patch simple - as is now. We can look for a > workqueue only if mptcp workqueue usage proves to be relevant in non > debug build. Thank you for the patch and the review! I just added this patch at the end of the series. - bcb252d3443b: mptcp: use mptcp worker for path management Tests and export are in progress. Cheers, Matt
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c index 7de09fdd42a3..a8ad20559aaa 100644 --- a/net/mptcp/pm.c +++ b/net/mptcp/pm.c @@ -10,8 +10,6 @@ #include <net/mptcp.h> #include "protocol.h" -static struct workqueue_struct *pm_wq; - /* path manager command handlers */ int mptcp_pm_announce_addr(struct mptcp_sock *msk, @@ -78,7 +76,7 @@ static bool mptcp_pm_schedule_work(struct mptcp_sock *msk, return false; msk->pm.status |= BIT(new_status); - if (queue_work(pm_wq, &msk->pm.work)) + if (schedule_work(&msk->work)) sock_hold((struct sock *)msk); return true; } @@ -181,35 +179,6 @@ int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc) return mptcp_pm_nl_get_local_id(msk, skc); } -static void pm_worker(struct work_struct *work) -{ - struct mptcp_pm_data *pm = container_of(work, struct mptcp_pm_data, - work); - struct mptcp_sock *msk = container_of(pm, struct mptcp_sock, pm); - struct sock *sk = (struct sock *)msk; - - lock_sock(sk); - spin_lock_bh(&msk->pm.lock); - - pr_debug("msk=%p status=%x", msk, pm->status); - if (pm->status & BIT(MPTCP_PM_ADD_ADDR_RECEIVED)) { - pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_RECEIVED); - mptcp_pm_nl_add_addr_received(msk); - } - if (pm->status & BIT(MPTCP_PM_ESTABLISHED)) { - pm->status &= ~BIT(MPTCP_PM_ESTABLISHED); - mptcp_pm_nl_fully_established(msk); - } - if (pm->status & BIT(MPTCP_PM_SUBFLOW_ESTABLISHED)) { - pm->status &= ~BIT(MPTCP_PM_SUBFLOW_ESTABLISHED); - mptcp_pm_nl_subflow_established(msk); - } - - spin_unlock_bh(&msk->pm.lock); - release_sock(sk); - sock_put(sk); -} - void mptcp_pm_data_init(struct mptcp_sock *msk) { msk->pm.add_addr_signaled = 0; @@ -223,22 +192,11 @@ void mptcp_pm_data_init(struct mptcp_sock *msk) msk->pm.status = 0; spin_lock_init(&msk->pm.lock); - INIT_WORK(&msk->pm.work, pm_worker); mptcp_pm_nl_data_init(msk); } -void mptcp_pm_close(struct mptcp_sock *msk) -{ - if (cancel_work_sync(&msk->pm.work)) - sock_put((struct sock *)msk); -} - void __init mptcp_pm_init(void) { - pm_wq = alloc_workqueue("pm_wq", WQ_UNBOUND | WQ_MEM_RECLAIM, 8); - if (!pm_wq) - panic("Failed to allocate workqueue"); - mptcp_pm_nl_init(); } diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index faa804f63c81..a349d8f06f20 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -1237,6 +1237,29 @@ static unsigned int mptcp_sync_mss(struct sock *sk, u32 pmtu) return 0; } +static void pm_work(struct mptcp_sock *msk) +{ + struct mptcp_pm_data *pm = &msk->pm; + + spin_lock_bh(&msk->pm.lock); + + pr_debug("msk=%p status=%x", msk, pm->status); + if (pm->status & BIT(MPTCP_PM_ADD_ADDR_RECEIVED)) { + pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_RECEIVED); + mptcp_pm_nl_add_addr_received(msk); + } + if (pm->status & BIT(MPTCP_PM_ESTABLISHED)) { + pm->status &= ~BIT(MPTCP_PM_ESTABLISHED); + mptcp_pm_nl_fully_established(msk); + } + if (pm->status & BIT(MPTCP_PM_SUBFLOW_ESTABLISHED)) { + pm->status &= ~BIT(MPTCP_PM_SUBFLOW_ESTABLISHED); + mptcp_pm_nl_subflow_established(msk); + } + + spin_unlock_bh(&msk->pm.lock); +} + static void mptcp_worker(struct work_struct *work) { struct mptcp_sock *msk = container_of(work, struct mptcp_sock, work); @@ -1253,6 +1276,9 @@ static void mptcp_worker(struct work_struct *work) __mptcp_flush_join_list(msk); __mptcp_move_skbs(msk); + if (msk->pm.status) + pm_work(msk); + if (test_and_clear_bit(MPTCP_WORK_EOF, &msk->flags)) mptcp_check_for_eof(msk); @@ -1444,7 +1470,6 @@ static void mptcp_close(struct sock *sk, long timeout) } mptcp_cancel_work(sk); - mptcp_pm_close(msk); __skb_queue_purge(&sk->sk_receive_queue); diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index 2c3deede2469..6d3fff97e3c6 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -174,8 +174,6 @@ struct mptcp_pm_data { u8 local_addr_max; u8 subflows_max; u8 status; - - struct work_struct work; }; struct mptcp_data_frag { @@ -417,7 +415,6 @@ void mptcp_crypto_hmac_sha(u64 key1, u64 key2, u8 *msg, int len, void *hmac); void __init mptcp_pm_init(void); void mptcp_pm_data_init(struct mptcp_sock *msk); -void mptcp_pm_close(struct mptcp_sock *msk); void mptcp_pm_new_connection(struct mptcp_sock *msk, int server_side); void mptcp_pm_fully_established(struct mptcp_sock *msk); bool mptcp_pm_allow_new_subflow(struct mptcp_sock *msk);
We can re-use the existing work queue to handle path management instead of a dedicated work queue. Just move pm_worker to protocol.c, call it from the mptcp worker and get rid of the msk lock (already held). Signed-off-by: Florian Westphal <fw@strlen.de> --- As a followup one could probably also merge 'status' and msk->flags, might also be able to remove the 'pending' boolean. I can have a look next week. net/mptcp/pm.c | 44 +------------------------------------------- net/mptcp/protocol.c | 27 ++++++++++++++++++++++++++- net/mptcp/protocol.h | 3 --- 3 files changed, 27 insertions(+), 47 deletions(-)