On 9/19/16, 7:42 PM, "osv-dev@googlegroups.com on behalf of Nadav Har'El"
<osv-dev@googlegroups.com on behalf of n...@scylladb.com> wrote:

>diff --git a/core/mempool.cc b/core/mempool.cc
>index 50c938f..fad046d 100644
>--- a/core/mempool.cc
>+++ b/core/mempool.cc
>@@ -1099,9 +1099,12 @@ struct l1 {
>     }
>     static void* alloc_page_local();
>     static bool free_page_local(void* v);
>-    void* pop() { return _pages[--nr]; }
>-    void push(void* page) { _pages[nr++] = page; }
>-    void* top() { return _pages[nr - 1]; }
>+    void* pop() {
>+        void *page = nullptr;
>
>
>
>
>Nitpick (not important compared to the bigger comments below):
>
>
>
>I think that setting this nullptr is redundant, and confusing (it led me
>to think there is a reason why it should be set.
>
>If you anyway leave this default value, you could do
>
>    _pages.pop(page);
>
>    return page;
>
>instead of the ?: expression below. Maybe that's what you intended to do?

Eh.  Just a habit of mine to always initialize variables.

>
>
>
>+        return (_pages.pop(page) ? page : nullptr);
>+    }
>+    bool push(void *page) { return _pages.push(page); }
>+    unsigned size() { return _pages.size(); }
>     void wake_thread() { _fill_thread.wake(); }
>     static void fill_thread();
>     static void refill();
>@@ -1110,11 +1113,12 @@ struct l1 {
>     static constexpr size_t max = 512;
>     static constexpr size_t watermark_lo = max * 1 / 4;
>     static constexpr size_t watermark_hi = max * 3 / 4;
>-    size_t nr = 0;
>
> private:
>     sched::thread _fill_thread;
>-    void* _pages[max];
>+
>+    typedef ring_spsc<void *, max> page_ring;
>
>
>+    page_ring _pages;
>
>
>
>
>I don't understand why you needed to change that array to a ring_spsc.

Yeah, I didn’t really need to.  The first thing I did when debugging this
was to switch the array
to a ring to absolutely prevent any possible overflow when handling pages.
 Then, I worked backward
from the assertion failures.  Once I couldn’t reproduce the issue, I just
kind of left
everything as-is.  I’ll clean it up and send a revised patch.

>@@ -1259,12 +1268,17 @@ void l1::refill()
> {
>     SCOPE_LOCK(preempt_lock);
>     auto& pbuf = get_l1();
>-    while (pbuf.nr <http://pbuf.nr> + page_batch::nr_pages < pbuf.max /
>2) {
>-        auto* pb = global_l2.alloc_page_batch(pbuf);
>-        if (pb) {
>+    auto* pb = global_l2.alloc_page_batch(pbuf);
>
>
>
>So alloc_page_batch dropped the preempt_lock momentarily (it did us the
>courtesy of preventing migration of the thread to another CPU, but I
>believe this is actually not necessary - we could have re-gotten get_l1()
>after the alloc_page_batch and not avoid
> migration).
>
>
>Wasn't the code here the real bug, and your fix here the real fix? I
>mean, won't this fix, of testing the room on the array again after
>calling alloc_page_batch(), work correctly without the rest of the
>changes to move to a ring_spsc?

Yes.

>
>
>By the way, looking at the (what I thought was unnecessary) migration
>lock in alloc_page_batch(), I am starting to think that the giving of
>pbuf to this function is unnecessary: It seems pbuf is only used there
>for some test looks completely unnecessary
> to me, now that anyway you do this test, more correctly, in the caller.
>But I guess we can consider this later.

Ok.  I’ll drop the argument from alloc_page_batch here.

>
>+    if (pb) {
>+        // Another thread might have filled the ring while we waited for
>+        // the page batch.  Make sure there is enough room to add the
>pages
>+        // we just acquired, otherwise return them.
>
>
>Note that the original code didn't intend to fill the entire array, but
>rather only half, but maybe it doesn't hurt using the entire batch if we
>already got it. So I think what you did is fine.
>
>
>
>+        if (pbuf.size() + page_batch::nr_pages <= pbuf.max) {
>             for (auto& page : pb->pages) {
>-                pbuf.push(page);
>+                assert(pbuf.push(page));
>

Cool, that was my thinking as well.

>
>
>I twinge when I see an assert with side-effects (as you know, other
>implementations like KASSERT for example, compile-out the entire assert
>on release builds). But this is actually fine with OSv's assert(). Still
>might be better style to rewrite this as
> two statements?
>
>

Noted.

>
>             }
>+        } else {
>+            global_l2.free_page_batch(pb);
>         }
>     }
> }
>@@ -1273,10 +1287,12 @@ void l1::unfill()
> {
>     SCOPE_LOCK(preempt_lock);
>     auto& pbuf = get_l1();
>-    while (pbuf.nr <http://pbuf.nr> > page_batch::nr_pages + pbuf.max /
>2) {
>-        auto* pb = static_cast<page_batch*>(pbuf.top());
>-        for (size_t i = 0 ; i < page_batch::nr_pages; i++) {
>-            pb->pages[i] = pbuf.pop();
>+    // Don't bother unfilling unless we are actually above our target
>size.
>+    if (pbuf.size() > pbuf.max / 2) {
>
>
>
>
>Here you changed the right hand side of the inequality a bit. Why?
>
>

Actually, I don’t remember.  Probably no good reason.

As I said, I’ll send an updated, more minimal patch soon.

Timmons





Spirent Communications e-mail confidentiality.
------------------------------------------------------------------------
This e-mail contains confidential and / or privileged information belonging to 
Spirent Communications plc, its affiliates and / or subsidiaries. If you are 
not the intended recipient, you are hereby notified that any disclosure, 
copying, distribution and / or the taking of any action based upon reliance on 
the contents of this transmission is strictly forbidden. If you have received 
this message in error please notify the sender by return e-mail and delete it 
from your system.

Spirent Communications plc
Northwood Park, Gatwick Road, Crawley, West Sussex, RH10 9XN, United Kingdom.
Tel No. +44 (0) 1293 767676
Fax No. +44 (0) 1293 767677

Registered in England Number 470893
Registered at Northwood Park, Gatwick Road, Crawley, West Sussex, RH10 9XN, 
United Kingdom.

Or if within the US,

Spirent Communications,
27349 Agoura Road, Calabasas, CA, 91301, USA.
Tel No. 1-818-676- 2300

-- 
You received this message because you are subscribed to the Google Groups "OSv 
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Reply via email to