[MPTCP] Re: [PATCH net-next 3/8] mptcp: update mptcp ack sequence from work queue

[MPTCP] Re: [PATCH net-next 3/8] mptcp: update mptcp ack sequence from work queue

[Paolo Abeni]

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

On Tue, 2020-02-25 at 13:05 +0100, Florian Westphal wrote:
> +static bool __mptcp_move_skbs(struct mptcp_sock *msk)
> +{
> +	unsigned int moved = 0;
> +	bool done;
> +
> +	do {
> +		struct sock *ssk = mptcp_subflow_recv_lookup(msk);
> +
> +		if (!ssk)
> +			break;
> +
> +		lock_sock(ssk);

I'm wondering if bh_lock_sock(ssk) would suffice here?

If so we could do the skbs move from mptcp_release_cb() in patch 8.

But it looks like the full lock_sock() is required by the skb_orphan()
call, which in turn modifies ssk->sk_forward_alloc, so no luck right?

Thanks!

Paolo

[MPTCP] Re: [PATCH net-next 3/8] mptcp: update mptcp ack sequence from work queue

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> > +	do {
> > +		struct sock *ssk = mptcp_subflow_recv_lookup(msk);
> > +
> > +		if (!ssk)
> > +			break;
> > +
> > +		lock_sock(ssk);
> 
> I'm wondering if bh_lock_sock(ssk) would suffice here?
> 
> If so we could do the skbs move from mptcp_release_cb() in patch 8.
> 
> But it looks like the full lock_sock() is required by the skb_orphan()
> call, which in turn modifies ssk->sk_forward_alloc, so no luck right?

Yep, the destructor of the skbs on the ssk rx queue assumes its called
with the ssk owned.

We could of course do bh_lock+then check 'owned' but I'm not sure its
worth it.

I'd suggest to keep the 'direct move from release_cb' in mind as
something we could try to do later.

When I tested an older version of this patch set on top of mptcp-next
then the number of times the work queue ran with nothing todo (no
data in any subflow, no retransmission pending, nothing to reclaim from
rtx queue etc. was really small -- a low 3-digit number for entire mptcp
selftest run iirc. (with poll).

[MPTCP] Re: [PATCH net-next 3/8] mptcp: update mptcp ack sequence from work queue

[Paolo Abeni]

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

On Tue, 2020-02-25 at 13:49 +0100, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > > +	do {
> > > +		struct sock *ssk = mptcp_subflow_recv_lookup(msk);
> > > +
> > > +		if (!ssk)
> > > +			break;
> > > +
> > > +		lock_sock(ssk);
> > 
> > I'm wondering if bh_lock_sock(ssk) would suffice here?
> > 
> > If so we could do the skbs move from mptcp_release_cb() in patch 8.
> > 
> > But it looks like the full lock_sock() is required by the skb_orphan()
> > call, which in turn modifies ssk->sk_forward_alloc, so no luck right?
> 
> Yep, the destructor of the skbs on the ssk rx queue assumes its called
> with the ssk owned.
> 
> We could of course do bh_lock+then check 'owned' but I'm not sure its
> worth it.

Yup, also I think is not trivial take the correct delayed action with
multiple subflows.

> I'd suggest to keep the 'direct move from release_cb' in mind as
> something we could try to do later.

Agreed! definitely not needed now - and possibly even not needed later,
given the complexity.

Thanks,

Paolo