Closed Bug 132208 Opened 18 years ago Closed 18 years ago

Implement PR_SendFile() with sendfile() and TCP_CORK

Categories

(NSPR :: NSPR, enhancement, P1)

4.1.2
x86
Linux
enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wtc, Assigned: wtc)

References

Details

Attachments

(2 files, 3 obsolete files)

Linux 2.2 kernel has the sendfile() system call and the
TCP_CORK socket option.  We should implement PR_SendFile()
with these.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → 4.2
Attached patch Proposed patch (obsolete) — Splinter Review
The patch has what appears to be an unrelated change to the way
file_nbytes_to_send is calculated in the HPUX variant.

The condition for setting TCP_CORK should instead be
 if (sfd->hlen != 0 || sfd->tlen != 0)

if (count != -1 && (flags & PR_TRANSMITFILE_CLOSE_SOCKET))
then I don't think you have to call setsockopt() to clear TCP_CORK. If the
kernel is implemented well, this could cause the FIN to be piggybacked on the
last data packet.  I don't know what the kernel's actual behavior is.
Attached patch Proposed patch (obsolete) — Splinter Review
Thank you for the code review, John.

You are right, there is an unrelated HP-UX change in the
previous patch.  I will put it in a separate patch.

I considered using
  if (sfd->hlen != 0 || sfd->tlen != 0)
as the condition for setting TCP_CORK.	I didn't do
that because both the sendfile(2) and tcp(7) man pages
only mention the use of TCP_CORK with headers but not
trailers:

SENDFILE(2)
       If you plan to use sendfile for sending	files  to  a  TCP
       socket,	but need to send some header data in front of the
       file contents, please see the TCP_CORK option in tcp(7) to
       minimize the number of packets and to tune performance.

TCP(7)
       TCP_CORK
	      If enabled don't	send  out  partial  frames.   All
	      queued  partial  frames are sent when the option is
	      cleared again.  This is useful for prepending head¡
	      ers  before  calling sendfile(2), or for throughput
	      optimization. This option cannot be  combined  with
	      TCP_NODELAY.

Any thoughts?  In this new patch, I use
  if (sfd->hlen != 0 || sfd->tlen != 0)
as you suggested.

As for your last suggestion:
  if (count != -1 && (flags & PR_TRANSMITFILE_CLOSE_SOCKET))
  then I don't think you have to call setsockopt() to clear
  TCP_CORK.
I did that originally but it led to a hang in our test on
Red Hat Linux 6.2.  The client thread in our test was hung
in PR_Recv, and the server thread had exited.  I have not
seen this hang on Red Hat Linux 7.2.  So it is possible
that the glibc or kernel in Red Hat Linux 7.2 does this
correctly.  But I will leave my code as is for NSPR 4.2 so
that it works on both Red Hat Linux 6.2 and 7.2 and we can
investigate the exact conditions under which we can skip
the "uncork" setsockopt call.
Attachment #75137 - Attachment is obsolete: true
This patch is the full implementation of PR_SendFile
using sendfile and TCP_CORK.

The algorithm to decide whether TCP_CORK should be
set is as follows.

We attempt to set TCP_CORK if there are header or
trailer data to be sent with the file.

However, if we detect that TCP_NODELAY has been set
on the socket, we do not set TCP_CORK.

If we set TCP_CORK, we remember to turn TCP_CORK off
before we return.
Attachment #75226 - Attachment is obsolete: true
Blocks: 133659
This is a new version of the patch.  I verified with experiments
that on Red Hat Linux 6.0, 6.2, 7.1, and 7.2, accepted sockets
inherit the TCP_NODELAY socket option of the listening socket.
So in pt_Accept I copy the 'tcp_nodelay' field of the listening
socket to the newly accepted socket.

I also added a warning log message if setsockopt(TCP_CORK) fails
with the EINVAL error.	This will help us find out if the socket
has the TCP_NODELAY socket option set by a non-NSPR function.
Attachment #75698 - Attachment is obsolete: true
Patch checked into the tip of NSPR.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.