Re: [PATCH net-next 2/7] rds: hold a sock ref from rds_message to the rds_sock

[Willem de Bruijn]

On Sun, Jan 28, 2018 at 5:18 PM, Sowmini Varadhan
 wrote:
> On (01/28/18 14:51), Willem de Bruijn wrote:
>> > On (01/25/18 15:44), Willem de Bruijn wrote:
>  ;
>> >> You may alos be able to do the same as tcp zerocopy and
>> >> hold an sk reference on the notification skb.
>  ;
>> I don't quite follow. Every notification skb is created when pages refcount
>> is increased. It persists until at least rds_rm_zerocopy_callback, after data
>> skb has been freed and pages refcount has been decreased.
>>
>> In this callback, skb is consumed if another skb is already queued on
>> the error queue, otherwise it is queued itself. It needs to hold a sock ref
>> until it can be queued.
>
> maybe I did not follow the original suggestion- were you
> suggesting that I hold a pointer to the sk from e.g., the skb->cb
> itself?

Yes, I mean associating the notification skb that is eventually
queued onto the error queue with the socket. For tcp zerocopy,
this happens implicitly in sock_omalloc.

> I dont know that it would make things simpler,
> whereas having the pointer and refcount in the rds_message itself,
> and track this independantly of whether/not zcopy was used, seems
> like a more consistent dsta-structure model, so I'd like to leave
> this as is.

Sounds good. You know the rds internals a lot better than I do,
so are the better judge on whether to choose that or sock_omalloc.

Re: [PATCH net-next 2/7] rds: hold a sock ref from rds_message to the rds_sock

[Sowmini Varadhan]

On (01/28/18 14:51), Willem de Bruijn wrote:
> > On (01/25/18 15:44), Willem de Bruijn wrote:
 ;
> >> You may alos be able to do the same as tcp zerocopy and
> >> hold an sk reference on the notification skb.
 ;
> I don't quite follow. Every notification skb is created when pages refcount
> is increased. It persists until at least rds_rm_zerocopy_callback, after data
> skb has been freed and pages refcount has been decreased.
> 
> In this callback, skb is consumed if another skb is already queued on
> the error queue, otherwise it is queued itself. It needs to hold a sock ref
> until it can be queued.

maybe I did not follow the original suggestion- were you 
suggesting that I hold a pointer to the sk from e.g., the skb->cb
itself? I dont know that it would make things simpler, 
whereas having the pointer and refcount in the rds_message itself,
and track this independantly of whether/not zcopy was used, seems 
like a more consistent dsta-structure model, so I'd like to leave 
this as is.

Re: [PATCH net-next 2/7] rds: hold a sock ref from rds_message to the rds_sock

[Willem de Bruijn]

On Thu, Jan 25, 2018 at 4:35 PM, Sowmini Varadhan
 wrote:
> On (01/25/18 15:44), Willem de Bruijn wrote:
>> > This patch manages the m_rs assignment in the rds_message with
>> > the necessary refcount book-keeping.
>>
>> You may alos be able to do the same as tcp zerocopy and
>> hold an sk reference on the notification skb.
>
> We tether the notification skb to the rds socket after the
> refcount on the rds_message goes to zero, so we already have a
> ref from the sk to the notification skb,
>
> If we kept a refcount of all notification skb's (even the ones that
> are not ready to be unpinned yet) on the sk, then we have additional
> complexity trying to figure out which skb's are ready for notification
> at any point, so not sure it would make things simpler..

I don't quite follow. Every notification skb is created when pages refcount
is increased. It persists until at least rds_rm_zerocopy_callback, after data
skb has been freed and pages refcount has been decreased.

In this callback, skb is consumed if another skb is already queued on
the error queue, otherwise it is queued itself. It needs to hold a sock ref
until it can be queued.

Re: [PATCH net-next 2/7] rds: hold a sock ref from rds_message to the rds_sock

[Sowmini Varadhan]

On (01/25/18 15:44), Willem de Bruijn wrote:
> > This patch manages the m_rs assignment in the rds_message with
> > the necessary refcount book-keeping.
> 
> You may alos be able to do the same as tcp zerocopy and
> hold an sk reference on the notification skb.

We tether the notification skb to the rds socket after the 
refcount on the rds_message goes to zero, so we already have a 
ref from the sk to the notification skb, 

If we kept a refcount of all notification skb's (even the ones that
are not ready to be unpinned yet) on the sk, then we have additional
complexity trying to figure out which skb's are ready for notification
at any point, so not sure it would make things simpler..

--Sowmini

Re: [PATCH net-next 2/7] rds: hold a sock ref from rds_message to the rds_sock

[Willem de Bruijn]

On Wed, Jan 24, 2018 at 12:45 PM, Sowmini Varadhan
 wrote:
> The existing model holds a reference from the rds_sock to the
> rds_message, but the rds_message does not itself hold a sock_put()
> on the rds_sock. Instead the m_rs field in the rds_message is
> assigned when the message is queued on the sock, and nulled when
> the message is dequeued from the sock.
>
> We want to be able to notify userspace when the rds_message
> is actually freed (from rds_message_purge(), after the refcounts
> to the rds_message go to 0). At the time that rds_message_purge()
> is called, the message is no longer on the rds_sock retransmit
> queue. Thus the explicit reference for the m_rs is needed to
> send a notification that will signal to userspace that
> it is now safe to free/reuse any pages that may have
> been pinned down for zerocopy.
>
> This patch manages the m_rs assignment in the rds_message with
> the necessary refcount book-keeping.

You may alos be able to do the same as tcp zerocopy and
hold an sk reference on the notification skb.