Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-Data: A9a23:bpprgq8fh9aiqbfFR6w0DrUDO3+TJUtcMsCJ2f8bNWPcYEJGY0x3z
 mofDDzQaPyPZ2GmKdpybovlpElTsMfXxocxSgo5/HhEQiMRo6IpJzg4wmQcnc+2BpeeJK6yx
 5xGMrEsCuhqFieEzvuKGue99iYUOZllwtMQMcacUsxLbVYMpBwJ1FQzxIbVvqYy2YLgWlrV5
 IupyyHiEAbNNwBcYzN8B52r9UsHUMTa4Fv0aXRnOJinFHeH/5UkJMp3yZOZdhMUcaEIdgKOf
 Nsv+Znilo/vE7XBPfv++lrzWhVirrc/pmFigFIOM0SpqkAqSiDfTs/XnRfTAKtao2zhojx/9
 DlCnYyNFxoyALTUouFDUxZfHw09ErcB24aSdBBTseTLp6HHW3DrxfNuCRlqe4Yf/OB6Cn0I6
 OMRND0XaheEwem/qF65YrA214Jyc4+xZNNZ5ioIITLxVJ7KRbjfT6jO5MRc0R8tmclSEOzaY
 M1fYjMHgBHoOUcQYgxMWcNWcOGAxST1Ug9foVCsnbM6/mbhyjZB06jUGY+AEjCNbYAP9qqCn
 UrE9mL/AjkVM9uQzTfD+XWp7sfGgyr0WYQ6G7q/+fpnxlaUwwQ7EBoNVnO0pv62jkP4UNVaQ
 2Qe4SchpKw23EOsSdb5Uluzp3vslgYeR/JfFOo17AzLwa3Riy6dB24ZXntIcN0OqsA7X3op2
 0WPktevAiZg2JWRSHSA5vKXoCm0NCw9M2APf2kHQBED7t2lp5s85jrLT9B+AOuwi/X2Bzj7w
 HaNtidWr7EOkckj1Kih+13DxTW2qfDhQgcr60PXV2S+4wVRYI+jepzu6F7H4PIGJ4GcJnGIv
 2ABs8yf6v0eSJ+KiSqBSfkMG7fv4OyKWBXAjlp/N50g8Smx4XmlfJAW7DwWDEJoNMkDUSXkb
 E/apUVa45o7FGOncaJtcaqwDcowxKTtHNijUerbBvJWZYNyXBeA5yIoZEn44oz2uEMrl6cyM
 ovdbNmlEXsADaNgijG/LwsA7YIWKukF7Tu7bfjGI96PiNJyuFb9pW85DWaz
Subject: Re: [PATCH v0.8 3/6] sched/umcg: implement UMCG syscalls
To:     Peter Oskolkov <posk@posk.io>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Andrew Morton <akpm@linux-foundation.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Andy Lutomirski <luto@kernel.org>, <linux-mm@kvack.org>,
        <linux-kernel@vger.kernel.org>, <linux-api@vger.kernel.org>
CC:     Paul Turner <pjt@google.com>, Ben Segall <bsegall@google.com>,
        Peter Oskolkov <posk@google.com>,
        Andrei Vagin <avagin@google.com>, Jann Horn <jannh@google.com>
References: <20211104195804.83240-1-posk@google.com>
 <20211104195804.83240-4-posk@google.com>
In-Reply-To: <20211104195804.83240-4-posk@google.com>
From:   Thierry Delisle <tdelisle@uwaterloo.ca>
Message-ID: <ec84f37d-da30-8f03-3864-0c94078f6e21@uwaterloo.ca>
Date:   Fri, 5 Nov 2021 19:48:21 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

On 2021-11-04 3:58 p.m., Peter Oskolkov wrote:
 > +/*
 > + * Try to wake up. May be called with preempt_disable set. May be called
 > + * cross-process.
 > + *
 > + * Note: umcg_ttwu succeeds even if ttwu fails: see wait/wake state
 > + *       ordering logic.
 > + */
 > +static int umcg_ttwu(u32 next_tid, int wake_flags)
 > +{
 > +    struct task_struct *next;
 > +
 > +    rcu_read_lock();
 > +    next = find_task_by_vpid(next_tid);
 > +    if (!next || !umcg_wakeup_allowed(next)) {
 > +        rcu_read_unlock();
 > +        return -ESRCH;
 > +    }
 > +
 > +    /* The result of ttwu below is ignored. */
 > +    try_to_wake_up(next, TASK_NORMAL, wake_flags);
 > +    rcu_read_unlock();
 > +
 > +    return 0;
 > +}

Doesn't try_to_wake_up return different values based on whether or not a 
task
was woken up? I think it could be useful to propagate that result instead of
always returning zero. Even if it only helps for debugging.


 > +static bool enqueue_idle_worker(struct umcg_task __user *ut_worker)
 > +{
 > +    u64 __user *node = &ut_worker->idle_workers_ptr;
 > +    u64 __user *head_ptr;
 > +    u64 first = (u64)node;
 > +    u64 head;
 > +
 > +    if (get_user(head, node) || !head)
 > +        return false;
 > +
 > +    head_ptr = (u64 __user *)head;
 > +
 > +    /* Mark the worker as pending. */
 > +    if (put_user(UMCG_IDLE_NODE_PENDING, node))
 > +        return false;
 > +
 > +    /* Make the head point to the worker. */
 > +    if (xchg_user_64(head_ptr, &first))
 > +        return false;
 > +
 > +    /* Make the worker point to the previous head. */
 > +    if (put_user(first, node))
 > +        return false;
 > +
 > +    return true;
 > +}

If the last two operation return false, whichever task tries to consume the
list could deadlock, depending on whether or not the ensuing
force_sig(SIGKILL); reaches the consuming task. Does the force_sig kill
the task or the entire process. Is it possible to consume this list from a
different process that shares the memory? I'm wondering if the last
two "return false" should attempt to retract the
UMCG_IDLE_NODE_PENDING.