mattiasflodin/reckless

[QUESTION] Claryfying the confusion in MPSC ring buffer initialization code

Closed this issue · 2 comments

nvevg commented

Hello, Mattias!

Thanks for your library, it is a great piece of work one could learn a lot from by reading the source code. I am studying the implementation of MPSC ring buffer now, and I am not sure I got everything right about mpsc_ring_buffer::init(std::size_t). Would you mind clarifying the confusion and also correct me if I got something wrong about the general idea of the following code?

capacity = round_capacity(capacity);

#if defined(__linux__)
    // allocate shared memory for buffer data
    int shm = shmget(IPC_PRIVATE, capacity, IPC_CREAT | S_IRUSR | S_IWUSR);
    if(shm == -1)
        throw std::bad_alloc();

    void* pbase = nullptr;
    while(!pbase) {
        // ensure we can acquire a set of consecutive pages twice as big as required capacity located
        // in process address space;
        pbase = mmap(nullptr, 2*capacity, PROT_NONE,
                MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
        if(MAP_FAILED == pbase)
            throw std::bad_alloc();
        // unmap it - we are not going to use it anyway
        munmap(pbase, 2*capacity);
        // attach shared segment to the location we got from mmap()
        pbase = shmat(shm, pbase, 0);
        // THAT IS WHERE IT FEELS OFF
        // shmat() returns either a pointer to attachment address or ((void *) -1), 
        // but does not return NULL in any case
        // Let's say someone had a chance to occupy the memory location we got from mmap() before we get here
        // (freshly loaded shared library etc.) so shmat() returned ((void *) -1)
        // SHOULDN'T IT BE if (pbase != (void*) -1) ?
        if(pbase) {
            // if first shmat() call succeded , map the same shared segment to the second half of page set
            if((void*)-1 == shmat(shm, static_cast<char*>(pbase) + capacity, 0))
            {
                shmdt(pbase);
                pbase = nullptr;
            }
        }
    }
    // mark segment for removal
    shmctl(shm, IPC_RMID, nullptr);
#elif defined(_WIN32)
// ...
#endif

Thank you again.

Hi,

You're very modest to frame this as just a question as if I could never make any mistakes. 😄 Looks to me as you've found a pretty glaring bug. The reason this came about, I suspect, is that the corresponding function in the Windows version of the code returns 0 on failure.

Feel free to submit a PR to correct this if you'd like. Otherwise I will make the necessary fix as soon as I have the opportunity.

Fixed in next release