Read-only cown v2 #45

vishalgupta97 · 2024-08-19T10:05:54Z

No description provided.

mjp41

Here are some initial comments on the PR. I'll do comments on the rest later

src/rt/sched/threadpool.h

src/rt/sched/behaviourcore.h

mjp41

Some more comments

src/rt/sched/behaviourcore.h

src/rt/sched/cown.h

mjp41 · 2024-09-18T14:12:46Z

src/rt/sched/cown.h

    }

+    // True means I can write,


Add comment that "Only one writer can call this."

marioskogias

Here are some comments

marioskogias · 2024-09-18T10:20:29Z

src/rt/sched/behaviourcore.h

+     */
+    bool set_next_slot_reader(Slot* n)
+    {
+      // Check that the last two bits are zero


It would be better to explain the semantic assertion you want to do and not the practical one.

marioskogias · 2024-09-18T10:21:30Z

src/rt/sched/behaviourcore.h

+     * Assumption - Cowns are allocated at 4 byte boundary. Last 2 bits are
+     * zero.
+     */
+    std::atomic<uintptr_t> _cown;


Is there a reason to change the naming convention? Would a wrapper type work better here?

marioskogias · 2024-09-18T10:23:04Z

src/rt/sched/behaviourcore.h

-      status.store(0, std::memory_order_relaxed);
+      return (
+        (status.load(std::memory_order_acquire) &
+         STATUS_NEXT_SLOT_READER_FLAG) == STATUS_NEXT_SLOT_READER_FLAG);


Do you need the equality check?

marioskogias · 2024-09-18T10:23:26Z

src/rt/sched/behaviourcore.h

+    bool is_wait_2pl()
+    {
+      return (_cown.load(std::memory_order_acquire) & COWN_2PL_READY_FLAG) !=
+        COWN_2PL_READY_FLAG;


Same as above. Do you need the check?

marioskogias · 2024-09-18T10:57:40Z

src/rt/sched/behaviourcore.h

+        return true;
+      }
+
+      uintptr_t old_status_val =


You probably need some more comments here. It is not clear why you add. Maybe say that this effectively is a fetch and or.

marioskogias · 2024-09-18T12:05:37Z

src/rt/sched/behaviourcore.h

@@ -392,20 +666,30 @@ namespace verona::rt
      // Sort the indexing array so we make the requests in the correct order


We need more comments in the schedule_many behaviour for the read only part

I think this comment is going to massively restructured by #49, so I would sooner leave that to there.

marioskogias · 2024-09-18T12:16:58Z

src/rt/sched/behaviourcore.h

-          std::get<0>(i) < std::get<0>(j) :
-          std::get<1>(i)->cown < std::get<1>(j)->cown;
+        if (std::get<1>(i)->cown() == std::get<1>(j)->cown())
+          if (std::get<0>(i) == std::get<0>(j))


I'm not sure I understand the comparison. Add a comment that describes the corner case with one bahaviour with the same cown once with read and once with write accesses.

marioskogias · 2024-09-18T12:36:19Z

src/rt/sched/behaviourcore.h

     */
    std::atomic<uintptr_t> status;

-    Slot(Cown* cown) : cown(cown), status(0) {}
+    static constexpr uintptr_t STATUS_SLOT_ACTIVE_FLAG = 0x1;


The term active needs better explanation and probably a different name. How about READ_RESOLVED?

marioskogias · 2024-09-18T12:54:24Z

src/rt/sched/behaviourcore.h


-    if (is_ready())
+    if (next_slot() == nullptr)


Maybe explain what this means since next_slot is overloaded, or use a method call? Maybe a no_successor() call?

Added a no_successors and used throughout.

marioskogias · 2024-09-18T13:05:03Z

src/rt/sched/behaviourcore.h

        Aal::pause();
      }
    }

-    assert(is_behaviour());
-    // Wake up the next behaviour.
+    if (is_read_only())


This is complicated and duplicates code. Would it be possible to simplify? Maybe add methodo to finish the read_only to remove duplicates.

Add a drop_read to slot to remove duplication.

…-format

* Add Data race detection to the example * Alter notification of next_writer Use the read_ref_count to carry whether there is a writer that is expected to be woken up once the readers complete.

vishalgupta97 requested a review from mjp41 August 19, 2024 10:05

vishalgupta97 changed the title ~~Read-only cown~~ Read-only cown v2 Aug 19, 2024

mjp41 reviewed Aug 19, 2024

View reviewed changes

mjp41 reviewed Aug 20, 2024

View reviewed changes

mjp41 reviewed Aug 21, 2024

View reviewed changes

src/rt/sched/behaviourcore.h Outdated Show resolved Hide resolved

src/rt/sched/behaviourcore.h Outdated Show resolved Hide resolved

src/rt/sched/behaviourcore.h Outdated Show resolved Hide resolved