[MPTCP] Re: [PATCH v3 5/9] Squash-to: "mptcp: Implement path manager interface commands"

[MPTCP] Re: [PATCH v3 5/9] Squash-to: "mptcp: Implement path manager interface commands"

[Matthieu Baerts]

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

Hi Paolo,

On 21/02/2020 19:38, Paolo Abeni wrote:
> On Fri, 2020-02-21 at 19:24 +0100, Matthieu Baerts wrote:
>> Hi Paolo,
>>
>> On 21/02/2020 17:48, Paolo Abeni wrote:
>>> Implement stubs for PM events delegating the action to the work
>>> queue. This allows acquiring whatever lock is needed to perform
>>> the actual implementation.
>>>
>>> Try to avoid scheduling the worker if no action is needed/possible.
>>> I relies on the accounting info included into the PM struct and
>>> on great deal of double-checked locking [anti-]pattern.
>>>
>>> v1  -> v2:
>>>    - fix several issues in mptcp_pm_addr_signal()
>>>
>>> RFC -> v1:
>>>    - simplify/cleanup mptcp_pm_work_pending() - Mat
>>>    - likewise simplify/cleanup mptcp_pm_add_addr()
>>>
>>> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
>>> ---
>>>    net/mptcp/pm.c | 223 +++++++++++++++++++------------------------------
>>>    1 file changed, 86 insertions(+), 137 deletions(-)
>>>
>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>>> index 30bce85adda1..d6e60b4bdb56 100644
>>> --- a/net/mptcp/pm.c
>>> +++ b/net/mptcp/pm.c
>>> @@ -209,7 +130,24 @@ void mptcp_pm_add_addr(struct mptcp_sock *msk,
>>>    bool mptcp_pm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>>>    			  struct mptcp_addr_info *saddr)
>>>    {
>>> -	return false;
>>> +	int ret = false;
>>> +
>>> +	spin_lock_bh(&msk->pm.lock);
>>> +
>>> +	/* double check after the lock is acquired */
>>> +	if (!mptcp_pm_should_signal(msk))
>>> +		goto out_unlock;
>>> +
>>> +	if (remaining < mptcp_add_addr_len(msk->pm.local.family))
>>> +		goto out_unlock;
>>> +
>>> +	*saddr = msk->pm.local;
>>> +	WRITE_ONCE(msk->pm.addr_signal, false);
>>> +	ret = true;
>>> +
>>> +out_unlock:
>>> +	spin_unlock_bh(&msk->pm.lock);
>>> +	return ret;
>>>    }
>>
>> I would still need to digest this new code (and look at the next
>> patches) but if a PM has to announce more than one ADD_ADDR, would it be
>> notified when the first one has been sent? To know when it can call
>> mptcp_pm_announce_addr() again.
> 
> Good question :) Currently we don't have yet the infrastructure for
> such notification. I think we could add that on top of reliable
> add_addr delivery: we can attach the next addr signaling to 'add_addr
> echo' opt reception.

Good idea! I forgot about the ADD_ADDRv1!

> I tried to keep the NL APIs/infrastructure generic enough to adjust the
> above.
That's good, thank you for that!

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

[MPTCP] Re: [PATCH v3 5/9] Squash-to: "mptcp: Implement path manager interface commands"

[Paolo Abeni]

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

On Fri, 2020-02-21 at 19:24 +0100, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 21/02/2020 17:48, Paolo Abeni wrote:
> > Implement stubs for PM events delegating the action to the work
> > queue. This allows acquiring whatever lock is needed to perform
> > the actual implementation.
> > 
> > Try to avoid scheduling the worker if no action is needed/possible.
> > I relies on the accounting info included into the PM struct and
> > on great deal of double-checked locking [anti-]pattern.
> > 
> > v1  -> v2:
> >   - fix several issues in mptcp_pm_addr_signal()
> > 
> > RFC -> v1:
> >   - simplify/cleanup mptcp_pm_work_pending() - Mat
> >   - likewise simplify/cleanup mptcp_pm_add_addr()
> > 
> > Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> > ---
> >   net/mptcp/pm.c | 223 +++++++++++++++++++------------------------------
> >   1 file changed, 86 insertions(+), 137 deletions(-)
> > 
> > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > index 30bce85adda1..d6e60b4bdb56 100644
> > --- a/net/mptcp/pm.c
> > +++ b/net/mptcp/pm.c
> > @@ -209,7 +130,24 @@ void mptcp_pm_add_addr(struct mptcp_sock *msk,
> >   bool mptcp_pm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >   			  struct mptcp_addr_info *saddr)
> >   {
> > -	return false;
> > +	int ret = false;
> > +
> > +	spin_lock_bh(&msk->pm.lock);
> > +
> > +	/* double check after the lock is acquired */
> > +	if (!mptcp_pm_should_signal(msk))
> > +		goto out_unlock;
> > +
> > +	if (remaining < mptcp_add_addr_len(msk->pm.local.family))
> > +		goto out_unlock;
> > +
> > +	*saddr = msk->pm.local;
> > +	WRITE_ONCE(msk->pm.addr_signal, false);
> > +	ret = true;
> > +
> > +out_unlock:
> > +	spin_unlock_bh(&msk->pm.lock);
> > +	return ret;
> >   }
> 
> I would still need to digest this new code (and look at the next 
> patches) but if a PM has to announce more than one ADD_ADDR, would it be 
> notified when the first one has been sent? To know when it can call 
> mptcp_pm_announce_addr() again.

Good question :) Currently we don't have yet the infrastructure for
such notification. I think we could add that on top of reliable
add_addr delivery: we can attach the next addr signaling to 'add_addr
echo' opt reception.

I tried to keep the NL APIs/infrastructure generic enough to adjust the
above.

Cheers,

Paolo

[MPTCP] Re: [PATCH v3 5/9] Squash-to: "mptcp: Implement path manager interface commands"

[Matthieu Baerts]

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

Hi Paolo,

On 21/02/2020 17:48, Paolo Abeni wrote:
> Implement stubs for PM events delegating the action to the work
> queue. This allows acquiring whatever lock is needed to perform
> the actual implementation.
> 
> Try to avoid scheduling the worker if no action is needed/possible.
> I relies on the accounting info included into the PM struct and
> on great deal of double-checked locking [anti-]pattern.
> 
> v1  -> v2:
>   - fix several issues in mptcp_pm_addr_signal()
> 
> RFC -> v1:
>   - simplify/cleanup mptcp_pm_work_pending() - Mat
>   - likewise simplify/cleanup mptcp_pm_add_addr()
> 
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> ---
>   net/mptcp/pm.c | 223 +++++++++++++++++++------------------------------
>   1 file changed, 86 insertions(+), 137 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 30bce85adda1..d6e60b4bdb56 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -209,7 +130,24 @@ void mptcp_pm_add_addr(struct mptcp_sock *msk,
>   bool mptcp_pm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
>   			  struct mptcp_addr_info *saddr)
>   {
> -	return false;
> +	int ret = false;
> +
> +	spin_lock_bh(&msk->pm.lock);
> +
> +	/* double check after the lock is acquired */
> +	if (!mptcp_pm_should_signal(msk))
> +		goto out_unlock;
> +
> +	if (remaining < mptcp_add_addr_len(msk->pm.local.family))
> +		goto out_unlock;
> +
> +	*saddr = msk->pm.local;
> +	WRITE_ONCE(msk->pm.addr_signal, false);
> +	ret = true;
> +
> +out_unlock:
> +	spin_unlock_bh(&msk->pm.lock);
> +	return ret;
>   }

I would still need to digest this new code (and look at the next 
patches) but if a PM has to announce more than one ADD_ADDR, would it be 
notified when the first one has been sent? To know when it can call 
mptcp_pm_announce_addr() again.

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium