Re: [PATCH] Remove unnecessary mmap in memory_map ipc

[Rongqing Li <rongqing.li@xxxxxxxxxxxxx>]

On 05/27/2013 11:39 PM, 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.

I guest, it works, because length of first mmap() is a
special value, it is cache aliasing. the return address of
mmap is aligned to the length, if the length is 8192, it will
not work.

./lib/util.h:#define IPC_DISPATCH_SIZE       1024*64
./lib/util.h:#define IPC_DISPATCH_SIZE       8192*128


-Roy

> > 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

--
Best Reagrds,
Roy | RongQing Li
_______________________________________________
discuss mailing list
discuss@xxxxxxxxxxxx
http://lists.corosync.org/mailman/listinfo/discuss