34920 - Provide an implementation of PR_AcceptRead, PR_TransmitFile, and PR_SendFile that can be used by I/O layer implementers

Status: RESOLVED FIXED

(NSPR :: NSPR, enhancement, P3)

Description

[Wan-Teh Chang]

NSPR defines three I/O methods, PR_AcceptRead,
PR_TransmitFile, and PR_SendFile, that are intended
to exploit fast system calls on some platforms,
for example, the AcceptEx and TransmitFile functions
on Windows NT.  However, only the bottom layer (i.e.,
the NSPR layer) can take advantage of these fast
system calls; all the upper layers need to emulate
these functions.  It turns out that the emulation of
these functions is the same for most upper I/O layers:
- PR_AcceptRead: PR_Accept followed by PR_Recv.
- PR_TransmitFile and PR_SendFile: memory-map the
  file, then write the memory-mapped region to the
  socket.

Instead of having each I/O layer emulate these I/O
methods, NSPR should provide the common, emulated
implementation of these I/O methods, which the I/O
layer implementers can just plug into the PRIOMethods
table of their layer.

Because normal NSPR clients don't call these
emulated functions directly, these emulated functions
should be exported as private functions and declared
in "private/pprio.h".

Comment 1

[Wan-Teh Chang]

Reassigned the bug to myself.

Comment 2

[Wan-Teh Chang]

I added two exported private functions (declared in pprio.h)
PR_EmulateAcceptRead and PR_EmulateSendFile that I/O layers
can use in their implementation of the acceptread and sendfile
methods.

PR_EmulateAcceptRead is based on the original internal function
_PR_EmulateAcceptRead, and PR_EmulateSendFile is based on the
original internal functions _PR_EmulateSendFile and
_PR_UnixSendFile.

I/O layers still need to define their transmitfile method in
terms of the sendfile method like this:
static PRInt32 foo_TransmitFile(
    PRFileDesc *sd, PRFileDesc *fd, const void *headers,
    PRInt32 hlen, PRTransmitFileFlags flags, PRIntervalTime timeout)
{
    PRSendFileData sfd;

    sfd.fd = fd;
    sfd.file_offset = 0;
    sfd.file_nbytes = 0;
    sfd.header = headers;
    sfd.hlen = hlen;
    sfd.trailer = NULL;
    sfd.tlen = 0;

    return foo_SendFile(sd, &sfd, flags, timeout);
}  /* foo_TransmitFile */

We should make this the default transmitfile method,
but I'm worried that it may break binary compatibility
so I didn't do that.

/cvsroot/mozilla/nsprpub/config/OSF1.mk, revision 3.10
/cvsroot/mozilla/nsprpub/pr/include/md/_hpux.h, revision 3.8
/cvsroot/mozilla/nsprpub/pr/include/private/pprio.h, revision 3.11
/cvsroot/mozilla/nsprpub/pr/include/private/primpl.h, revision 3.41
/cvsroot/mozilla/nsprpub/pr/src/io/priometh.c, revision 3.7
/cvsroot/mozilla/nsprpub/pr/src/io/prsocket.c, revision 3.33
/cvsroot/mozilla/nsprpub/pr/src/md/unix/unix.c, revision 3.35
/cvsroot/mozilla/nsprpub/pr/src/pthreads/ptio.c, revision 3.51

Comment 3

[Wan-Teh Chang]

Found a bug during testing on NT.  Reopened the bug.

Comment 4

[Wan-Teh Chang]

The bug I found has to do with the alignment of
the offset argument to PR_MemMap.  Right now we
assume offset must be aligned on page boundaries.
Turns out that this is true on Unix but not true
on Windows.

The Unix mmap man page says:
    pa=mmap(addr, len, prot, flags, fildes, off);
    [...]
    The off argument is constrained to be aligned
    and sized according to the value returned by
    sysconf() when passed _SC_PAGESIZE or _SC_PAGE_SIZE.
    [...] The implementation performs mapping operations
    over whole pages. Thus, while the argument len need
    not meet a size or alignment constraint, the
    implementation will include, in any mapping operation,
    any partial page specified by the range [pa, pa + len).
(http://www.opengroup.org/onlinepubs/007908799/xsh/mmap.html)

The Win32 MapViewOfFile documentation says:
dwFileOffsetHigh 
     [in] Specifies the high-order DWORD of the file offset
     where mapping is to begin. 
dwFileOffsetLow 
     [in] Specifies the low-order DWORD of the file offset
     where mapping is to begin. The combination of the high
     and low offsets must specify an offset within the file
     that matches the system's memory allocation granularity,
     or the function fails. That is, the offset must be a
     multiple of the allocation granularity. Use the
     GetSystemInfo function, which fills in the members of a
     SYSTEM_INFO structure, to obtain the system's memory
     allocation granularity. 
(http://msdn.microsoft.com/library/psdk/winbase/filemap_8p9h.htm)

On NT 4.0, the page size is 4K whereas the allocation
granularity is 64K.  If offset is a multiple of page
size but not a multiple of the allocation granularity,
MapViewOfFile fails with ERROR_MAPPED_ALIGNMENT (1132L).

We should add a new function PR_GetMemMapAlignment
that returns the alignment of the offset argument to
PR_MemMap.

Comment 5

[Wan-Teh Chang]

Fixed PR_EmulateSendFile to use the new PR_GetMemMapAlignment
function to get the proper alignment of the offset argument
to PR_MemMap.  Also, the size argument to PR_CreateFileMap
and the offset argument to PR_MemMap are 64-bit, so use
64-bit variables and LL macros.

/cvsroot/mozilal/nsprpub/pr/src/io/priometh.c, revision 3.8

Comment 6

[Wan-Teh Chang]

Modified PR_EmulateSendFile so that it doesn't
depend on the mmap alignment being a power of 2.

/cvsroot/mozilla/nsprpub/pr/src/io/priometh.c, revision 3.9

Comment 7

[Wan-Teh Chang]

Modified the socket.c test to test PR_EmulateSendFile.
/cvsroot/mozilla/nsprpub/pr/tests/socket.c, revision 3.15

Added the new test acceptreademu.c to test PR_EmulateAcceptRead.

Marked the bug fixed.

Comment 8

[Wan-Teh Chang]

I found a bug in PR_EmulateAcceptRead() that is
manifested on some 64-bit platforms.

I define
     enum { AMASK = 7 };
and bitwise-AND an address with ~AMASK.  If
an enum is implemented as an unsigned int (32-bit),
~AMASK is still unsigned, so when extended to 64-bit
(0x00000000fffffff8) and bitwise-ANDed with an address,
the upper 32 bits of the address are zeroed.

This bug won't be noticed if enum is implemented as
a signed int, because ~AMASK is signed-extended to
be 0xfffffffffffffff8.

The fix is to explicitly define AMASK as a PRPtrdiff value:
Index: mozilla/nsprpub/pr/src/io/priometh.c
===================================================================
RCS file: /cvsroot/mozilla/nsprpub/pr/src/io/priometh.c,v
retrieving revision 3.13
diff -u -r3.13 priometh.c
--- priometh.c  2000/07/19 22:03:28     3.13
+++ priometh.c  2000/08/10 02:25:20
@@ -307,7 +307,7 @@
     if (rv >= 0)
     {
         /* copy the new info out where caller can see it */
-        enum { AMASK = 7 };  /* mask for alignment of PRNetAddr */
+#define AMASK ((PRPtrdiff)7)  /* mask for alignment of PRNetAddr */
         PRPtrdiff aligned = (PRPtrdiff)buf + amount + AMASK;
         *raddr = (PRNetAddr*)(aligned & ~AMASK);
         memcpy(*raddr, &remote, PR_NETADDR_SIZE(&remote));

I checked in the fix.
/cvsroot/mozilla/nsprpub/pr/src/io/priometh.c, revision: 3.14

Larry, this explained the accept, acceptread, and acceptreademu
test failures on 64-bit HP-UX and Solaris.

Comment 9

[Wan-Teh Chang]

I said:
> Larry, this explained the accept, acceptread, and acceptreademu
> test failures on 64-bit HP-UX and Solaris.

It should be just 64-bit HP-UX.  The failure of those tests on
64-bit Solaris was caused by something else.

Comment 10

[larryh (gone)]

Good bug shooting!
The Solaris problems, I believe, were a procedural error in running the tests 
concurrently. ... I'll mergeout and re-test.