Re: [PATCH net-next 2/7] rds: hold a sock ref from rds_message to the rds_sock
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
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
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
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
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.