Re: [PATCH] Remove unnecessary mmap in memory_map ipc

[Steven Dake <sdake@xxxxxxxxxx>]

On 05/27/2013 08:39 AM, Jan Friesse wrote:
> Rongqing Li napsal(a):
> > This patch fixed the bug on MIPS, and works well.
> Rongqing,
> I'm not sure WHY circular_memory_map (or 3 mmaps used there) works, but
> as long as it works, I would probably keep it as it is.
Honza,

1. The first mmap maps an address (like malloc) that is 2x the size of 
the memory buffer.

2. The second maps a memory buffer into the first block of memory done 
in step 1.

3. The third mmap memory maps that same buffer into the second block of 
memory in step 1.

This results in a buffer that looks like this

|BUFFER|BUFFER|

If a read/write occurs at buffer 1 R, it will continue on into buffer 2 
BUFF, removing the need for special logic to handle end of buffer 
read/write conditions.

This dramatically improved reliability and performance, I suppose at the 
cost of portability to some non-cache-coherent hardware.

It may be possible to add a portability mode that doesn't use the mmap 
(and instead calculates and copies only blocks it needs) but I'm not 
quite sure how that would operate and seems to be at risk of regression.

Regards
-steve

> > But I have a question, should we do the same change in
> > circular_memory_map() ? which are using mmap() three times.
> >
> >
> > -RongQing
> >
> >
> > On 05/23/2013 03:25 AM, Fabio M. Di Nitto wrote:
> > > Looks good.
> > >
> > > Fabio
> > >
> > > On 05/22/2013 05:53 PM, Jan Friesse wrote:
> > > > This is similar patch as master e684e4ca6fed709c14d79d8d81f254aa48e1c65a
> > > > but for whole IPC.
> > > >
> > > > Signed-off-by: Jan Friesse <jfriesse@xxxxxxxxxx>
> > > > ---
> > > >    exec/coroipcs.c |   17 ++++-------------
> > > >    lib/coroipcc.c  |   17 +++++------------
> > > >    2 files changed, 9 insertions(+), 25 deletions(-)
> > > >
> > > > diff --git a/exec/coroipcs.c b/exec/coroipcs.c
> > > > index 7645499..39f7ff7 100644
> > > > --- a/exec/coroipcs.c
> > > > +++ b/exec/coroipcs.c
> > > > @@ -222,7 +222,6 @@ memory_map (
> > > >        void **buf)
> > > >    {
> > > >        int32_t fd;
> > > > -    void *addr_orig;
> > > >        void *addr;
> > > >        int32_t res;
> > > >
> > > > @@ -239,18 +238,10 @@ memory_map (
> > > >            goto error_close_unlink;
> > > >        }
> > > >
> > > > -    addr_orig = mmap (NULL, bytes, PROT_NONE,
> > > > -        MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> > > > +    addr = mmap (NULL, bytes, PROT_READ | PROT_WRITE,
> > > > +        MAP_SHARED, fd, 0);
> > > >
> > > > -    if (addr_orig == MAP_FAILED) {
> > > > -        goto error_close_unlink;
> > > > -    }
> > > > -
> > > > -    addr = mmap (addr_orig, bytes, PROT_READ | PROT_WRITE,
> > > > -        MAP_FIXED | MAP_SHARED, fd, 0);
> > > > -
> > > > -    if (addr != addr_orig) {
> > > > -        munmap(addr_orig, bytes);
> > > > +    if (addr == MAP_FAILED) {
> > > >            goto error_close_unlink;
> > > >        }
> > > >    #if (defined COROSYNC_BSD && defined MADV_NOSYNC)
> > > > @@ -261,7 +252,7 @@ memory_map (
> > > >        if (res) {
> > > >            return (-1);
> > > >        }
> > > > -    *buf = addr_orig;
> > > > +    *buf = addr;
> > > >        return (0);
> > > >
> > > >    error_close_unlink:
> > > > diff --git a/lib/coroipcc.c b/lib/coroipcc.c
> > > > index 140fa18..b3a074f 100644
> > > > --- a/lib/coroipcc.c
> > > > +++ b/lib/coroipcc.c
> > > > @@ -405,7 +405,6 @@ static int
> > > >    memory_map (char *path, const char *file, void **buf, size_t bytes)
> > > >    {
> > > >        int32_t fd;
> > > > -    void *addr_orig;
> > > >        void *addr;
> > > >        int32_t res;
> > > >        char *buffer;
> > > > @@ -451,28 +450,22 @@ retry_write:
> > > >        }
> > > >        free (buffer);
> > > >
> > > > -    addr_orig = mmap (NULL, bytes, PROT_NONE,
> > > > -        MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> > > > -
> > > > -    if (addr_orig == MAP_FAILED) {
> > > > -        goto error_close_unlink;
> > > > -    }
> > > >
> > > > -    addr = mmap (addr_orig, bytes, PROT_READ | PROT_WRITE,
> > > > -        MAP_FIXED | MAP_SHARED, fd, 0);
> > > > +    addr = mmap (NULL, bytes, PROT_READ | PROT_WRITE,
> > > > +        MAP_SHARED, fd, 0);
> > > >
> > > > -    if (addr != addr_orig) {
> > > > +    if (addr == MAP_FAILED) {
> > > >            goto error_close_unlink;
> > > >        }
> > > >    #if (defined COROSYNC_BSD && defined MADV_NOSYNC)
> > > > -    madvise(addr_orig, bytes, MADV_NOSYNC);
> > > > +    madvise(addr, bytes, MADV_NOSYNC);
> > > >    #endif
> > > >
> > > >        res = close (fd);
> > > >        if (res) {
> > > >            return (-1);
> > > >        }
> > > > -    *buf = addr_orig;
> > > > +    *buf = addr;
> > > >
> > > >        return 0;
> > > _______________________________________________
> > > discuss mailing list
> > > discuss@xxxxxxxxxxxx
> > > http://lists.corosync.org/mailman/listinfo/discuss
> _______________________________________________
> discuss mailing list
> discuss@xxxxxxxxxxxx
> http://lists.corosync.org/mailman/listinfo/discuss

_______________________________________________
discuss mailing list
discuss@xxxxxxxxxxxx
http://lists.corosync.org/mailman/listinfo/discuss