On Tue, Jul 24, 2018 at 05:13:26AM +0000, Vakul Garg wrote: > > > -----Original Message----- > > From: Doron Roberts-Kedes [mailto:doro...@fb.com] > > Sent: Tuesday, July 24, 2018 3:50 AM > > @@ -811,6 +809,7 @@ int tls_sw_recvmsg(struct sock *sk, > > likely(!(flags & MSG_PEEK))) { > > struct scatterlist sgin[MAX_SKB_FRAGS + 1]; > > int pages = 0; > > + int orig_chunk = chunk; > > > > zc = true; > > sg_init_table(sgin, MAX_SKB_FRAGS + 1); > > @@ -820,9 +819,11 @@ int tls_sw_recvmsg(struct sock *sk, > > err = zerocopy_from_iter(sk, &msg- > > >msg_iter, > > to_copy, &pages, > > &chunk, &sgin[1], > > - MAX_SKB_FRAGS, > > false, true); > > - if (err < 0) > > + MAX_SKB_FRAGS, > > false); > > + if (err < 0) { > > + iov_iter_revert(&msg->msg_iter, > > chunk - orig_chunk); > > goto fallback_to_reg_recv; > > + } > > This assumes that msg_iter gets advanced even if zerocopy_from_iter() fails.
Not sure I see what you mean. If msg_iter is not advanced then chunk - orig_chunk is 0, and the revert is a no-op. > It is easier from code readability perspective if functions upon failure do > not leave any side effects for the caller to clean-up. > I suggest that iov_iter_revert() should be called from zerocopy_from_iter() > itself if it is going to fail. Agreed that zerocopy_from_iter() should call iov_iter_revert(). I didn't do that because at first glace, the tx path seems to depend on the iov_iter_revert() being called as a result of either failed zerocopy_from_iter() or tls_push_record(). However, I think the latter path cannot actually be taken because tls_push_record appears never to return a positive value. This means that the code between the continue and fallback_to_reg_send should probably just be replaced with simply, goto send_end. After that change, its clear that iov_iter_revert() can be safely moved inside zerocopy_from_iter() in the error case. Pending your input, I'll plan on putting up a 2 part patch addressing each of these.
