Peter Zijlstra <[email protected]> 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
