On Thu, Dec 20, 2012 at 11:44 PM, Vicente J. Botet Escriba <vicente.botet@wanadoo.fr> wrote:

Le 20/12/12 17:24, Fredrik Orderud a écrit :

Attached is an early "draft" implementation of "shared_pri_mutex" with support for the following prioritization policies: UNSPECIFIED_PRIORITY, EXCLUSIVE_PRIORITY and SHARED_PRIORITY. Associated test code is attached in main.cpp. The implementation is based on a stripped-down version of boost/thread/pthread/shared_mutex.hpp, and implements policies through an enum template argument.

This is a good starting point. Do you expect that shared_pri_mutex< UNSPECIFIED_PRIORITY> behaves as shared_mutex?

Yes, that was the idea. Note that my internal state_data struct does differ a bit from the ones in the win32 & pthread shared_mutex implementations, since I couldn't fully understand them. In particular, I've extended the 1-bit/bool exclusive_waiting_blocked flag into an exclusive_request_count counter.

Questions:

* Are you aware of any documentation for the shared_mutex locking algorithm? In particular, I'm curious about the precise workings of exclusive_waiting_blocked.

* Why does state_data differ so much between the pthread and win32 implementations? In particular, why are the shared_waiting/exclusive_waiting counters only present in the win32 impl.? Does this mean that win32 vs. pthread shared_mutex behaves differently?

* The win32 imp. appears to be "better" in that it's lockless (except for semaphores), whereas pthread has an internal mutex protecting all methods (with condition variables). Are there any specific reasons why the pthread imp. isn't also lockless?

I think it could be beneficial to first take some steps towards unifying/deduplicating the shared_mutex implementations before adding more "priority policy" complexity into the mix.

The policies could probably also be implemented with a class hierarchy if preferred.

The enum seems ok to me, but maybe, having specific classes at the user level could introduce less constraints: shared_mutex, shared_mutex_prio_shared, , shared_mutex_prio_exclusive. This doesn't means that the implementation could not use a unique implementation as far as there is no loss on performances.

Agree.

Could you (or someone else) please provide some feedback on the "shared_pri_mutex" concept, and realism of getting a revised/extended implementation incorporated into boost::thread at some point?

I would prefer to integrate into Boost an implementation that takes in account upgrade-locking and timed operations. As soon as we have a complete and correct implementation, with enough test and documentation we could integrate them in a release. The documentation will need a motivation, why do we need them? If you can work on some of these subjects the integration could be do sooner.

Agree. I just need some time to dig into the details on the existing implementations.

Anyway, for the time been I have some minor remarks:

* the enum literals should be in lowercase and I would prefer a enum class.

    //enum class shared_mutex_priority
    BOOST_SCOPED_ENUM_DECLARE_BEGIN(shared_mutex_priority)
    {
        unspecified,
        exclusive,
        shared
    }

* mutex are not copiable

        BOOST_THREAD_NO_COPYABLE(shared_pri_mutex)

* protect the use of interruptions with BOOST_THREAD_PROVIDES_INTERRUPTIONS as in trunk

#if defined BOOST_THREAD_PROVIDES_INTERRUPTIONS
            boost::this_thread::disable_interruption do_not_disturb;
#endif

* boost::mutex::scoped_lock is deprecated
Use
            boost::unique_lock<boost::mutex> lk(state_change);

instead of

             boost::mutex::scoped_lock lk(state_change);

as in trunk.

Fully agree with you comments. Consider them "done". :-)

Maybe it is worth creating a Trac ticket.

That might be, but I will first need to spend some time first to implement a proper proposal that also incorporates the "upgrade" aspect. This will take some time.

Thanks for all the constructive comments Vicente.

Regards,

Fredrik