lkml.org 
[lkml]   [2019]   [Aug]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH RFC 2/2] futex: Implement mechanism to wait on any of several futexes
Date
Peter Zijlstra <peterz@infradead.org> writes:

>
>> +static int futex_wait_multiple(u32 __user *uaddr, unsigned int flags,
>> + u32 count, ktime_t *abs_time)
>> +{
>> + struct futex_wait_block *wb;
>> + struct restart_block *restart;
>> + int ret;
>> +
>> + if (!count)
>> + return -EINVAL;
>> +
>> + wb = kcalloc(count, sizeof(struct futex_wait_block), GFP_KERNEL);
>> + if (!wb)
>> + return -ENOMEM;
>> +
>> + if (copy_from_user(wb, uaddr,
>> + count * sizeof(struct futex_wait_block))) {
>> + ret = -EFAULT;
>> + goto out;
>> + }
>
> I'm thinking we can do away with this giant copy and do it one at a time
> from the other function, just extend the storage allocated there to
> store whatever values are still required later.

Hey Peter,

Thanks for your very detailed review. it is deeply appreciated. My
apologies for the style issues, I blindly trusted checkpatch.pl, when it
said it was ready for submission.

I'm not sure I get the suggestion here. If I understand the code
correctly, once we do it one at a time, we need to queue_me() each futex
and then drop the hb lock, before going to the next one. Once we go to
the next one, we need to call get_user_pages (and now copy_from_user),
both of which can sleep, and on return set the task state to
TASK_RUNNING. This opens a window where we can wake up the task but it
is not in the right sleeping state, which from the comment in
futex_wait_queue_me(), seems problematic. This is also the reason why I
wanted to split the key memory pin from the actual read in patch 1/2.

Did you consider this problem or is it not a problem for some reason?
What am I missing?

--
Gabriel Krisman Bertazi

\
 
 \ /
  Last update: 2019-08-06 08:26    [W:0.037 / U:4.816 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site