[MPTCP] Re: [PATCH mptcp-next] mptcp: use mptcp worker for path management

[MPTCP] Re: [PATCH mptcp-next] mptcp: use mptcp worker for path management

[Florian Westphal]

[-- Attachment #1: Type: text/plain, Size: 295 bytes --]

Paolo Abeni <pabeni(a)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?

[MPTCP] Re: [PATCH mptcp-next] mptcp: use mptcp worker for path management

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 1362 bytes --]

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(a)redhat.com> wrote:
>>> On Mon, 2020-06-22 at 13:18 +0200, Florian Westphal wrote:
>>>> Paolo Abeni <pabeni(a)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
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

[MPTCP] Re: [PATCH mptcp-next] mptcp: use mptcp worker for path management

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]

On Mon, 2020-06-22 at 15:03 +0200, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > On Mon, 2020-06-22 at 13:18 +0200, Florian Westphal wrote:
> > > Paolo Abeni <pabeni(a)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

[MPTCP] Re: [PATCH mptcp-next] mptcp: use mptcp worker for path management

[Florian Westphal]

[-- Attachment #1: Type: text/plain, Size: 812 bytes --]

Paolo Abeni <pabeni(a)redhat.com> wrote:
> On Mon, 2020-06-22 at 13:18 +0200, Florian Westphal wrote:
> > Paolo Abeni <pabeni(a)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?)

[MPTCP] Re: [PATCH mptcp-next] mptcp: use mptcp worker for path management

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 641 bytes --]

On Mon, 2020-06-22 at 13:18 +0200, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)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

[MPTCP] Re: [PATCH mptcp-next] mptcp: use mptcp worker for path management

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 2024 bytes --]

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