Hi Avi,
Today's linux-next merge of the kvm tree got a conflict in fs/eventfd.c
between commit 133890103b9de08904f909995973e4b5c08a780e ("eventfd:
revised interface and cleanups") from Linus' tree and commit
28ddf0aebbf546e56efd1951725d5457ce1ebf98 ("eventfd: Allow waiters to be
notified about the eventfd file* going away") from the kvm tree.
Overlapping changes. I fixed it up (see below), but don't know if this
is the correct fix.
--
Cheers,
Stephen Rothwell [email protected]
diff --cc fs/eventfd.c
index 31d12de,72f5f8d..0000000
--- a/fs/eventfd.c
+++ b/fs/eventfd.c
@@@ -105,8 -63,13 +105,13 @@@ static int eventfd_release(struct inod
{
struct eventfd_ctx *ctx = file->private_data;
- wake_up_poll(&ctx->wqh, POLLHUP);
+ /*
+ * No need to hold the lock here, since we are on the file cleanup
+ * path and the ones still attached to the wait queue will be
+ * serialized by wake_up_locked_poll().
+ */
+ wake_up_locked_poll(&ctx->wqh, POLLHUP);
- kfree(ctx);
+ eventfd_ctx_put(ctx);
return 0;
}
On Wed, 1 Jul 2009, Stephen Rothwell wrote:
> Hi Avi,
>
> Today's linux-next merge of the kvm tree got a conflict in fs/eventfd.c
> between commit 133890103b9de08904f909995973e4b5c08a780e ("eventfd:
> revised interface and cleanups") from Linus' tree and commit
> 28ddf0aebbf546e56efd1951725d5457ce1ebf98 ("eventfd: Allow waiters to be
> notified about the eventfd file* going away") from the kvm tree.
>
> Overlapping changes. I fixed it up (see below), but don't know if this
> is the correct fix.
>
> --
> Cheers,
> Stephen Rothwell [email protected]
>
> diff --cc fs/eventfd.c
> index 31d12de,72f5f8d..0000000
> --- a/fs/eventfd.c
> +++ b/fs/eventfd.c
> @@@ -105,8 -63,13 +105,13 @@@ static int eventfd_release(struct inod
> {
> struct eventfd_ctx *ctx = file->private_data;
>
> - wake_up_poll(&ctx->wqh, POLLHUP);
> + /*
> + * No need to hold the lock here, since we are on the file cleanup
> + * path and the ones still attached to the wait queue will be
> + * serialized by wake_up_locked_poll().
> + */
> + wake_up_locked_poll(&ctx->wqh, POLLHUP);
> - kfree(ctx);
> + eventfd_ctx_put(ctx);
> return 0;
> }
That's fine.
There are a couple of extra spaces before the last two -+ in that patch
though ;)
- Davide
On 07/01/2009 10:10 AM, Davide Libenzi wrote:
> On Wed, 1 Jul 2009, Stephen Rothwell wrote:
>
>
>> Hi Avi,
>>
>> Today's linux-next merge of the kvm tree got a conflict in fs/eventfd.c
>> between commit 133890103b9de08904f909995973e4b5c08a780e ("eventfd:
>> revised interface and cleanups") from Linus' tree and commit
>> 28ddf0aebbf546e56efd1951725d5457ce1ebf98 ("eventfd: Allow waiters to be
>> notified about the eventfd file* going away") from the kvm tree.
>>
>> Overlapping changes. I fixed it up (see below), but don't know if this
>> is the correct fix.
>>
>> --
>> Cheers,
>> Stephen Rothwell [email protected]
>>
>> diff --cc fs/eventfd.c
>> index 31d12de,72f5f8d..0000000
>> --- a/fs/eventfd.c
>> +++ b/fs/eventfd.c
>> @@@ -105,8 -63,13 +105,13 @@@ static int eventfd_release(struct inod
>> {
>> struct eventfd_ctx *ctx = file->private_data;
>>
>> - wake_up_poll(&ctx->wqh, POLLHUP);
>> + /*
>> + * No need to hold the lock here, since we are on the file cleanup
>> + * path and the ones still attached to the wait queue will be
>> + * serialized by wake_up_locked_poll().
>> + */
>> + wake_up_locked_poll(&ctx->wqh, POLLHUP);
>> - kfree(ctx);
>> + eventfd_ctx_put(ctx);
>> return 0;
>> }
>>
>
> That's fine.
> There are a couple of extra spaces before the last two -+ in that patch
> though ;)
>
No, that's a git N-way diff format. The first column shows the changes
relative to mainline by kvm.git, and the second the changes to kvm.git
made by mainline.
I've merged and will push soon, which will resolve the conflict, but I
think the patch wake_up_locked_poll() is better off in mainline rather
than kvm.git.
--
error compiling committee.c: too many arguments to function
Avi Kivity wrote:
> On 07/01/2009 10:10 AM, Davide Libenzi wrote:
>> On Wed, 1 Jul 2009, Stephen Rothwell wrote:
>>
>>
>>> Hi Avi,
>>>
>>> Today's linux-next merge of the kvm tree got a conflict in fs/eventfd.c
>>> between commit 133890103b9de08904f909995973e4b5c08a780e ("eventfd:
>>> revised interface and cleanups") from Linus' tree and commit
>>> 28ddf0aebbf546e56efd1951725d5457ce1ebf98 ("eventfd: Allow waiters to be
>>> notified about the eventfd file* going away") from the kvm tree.
>>>
>>> Overlapping changes. I fixed it up (see below), but don't know if this
>>> is the correct fix.
>>>
>>> --
>>> Cheers,
>>> Stephen Rothwell [email protected]
>>>
>>> diff --cc fs/eventfd.c
>>> index 31d12de,72f5f8d..0000000
>>> --- a/fs/eventfd.c
>>> +++ b/fs/eventfd.c
>>> @@@ -105,8 -63,13 +105,13 @@@ static int eventfd_release(struct inod
>>> {
>>> struct eventfd_ctx *ctx = file->private_data;
>>>
>>> - wake_up_poll(&ctx->wqh, POLLHUP);
>>> + /*
>>> + * No need to hold the lock here, since we are on the file
>>> cleanup
>>> + * path and the ones still attached to the wait queue will be
>>> + * serialized by wake_up_locked_poll().
>>> + */
>>> + wake_up_locked_poll(&ctx->wqh, POLLHUP);
>>> - kfree(ctx);
>>> + eventfd_ctx_put(ctx);
>>> return 0;
>>> }
>>>
>>
>> That's fine.
>> There are a couple of extra spaces before the last two -+ in that patch
>> though ;)
>>
>
> No, that's a git N-way diff format. The first column shows the
> changes relative to mainline by kvm.git, and the second the changes to
> kvm.git made by mainline.
>
> I've merged and will push soon, which will resolve the conflict, but I
> think the patch wake_up_locked_poll() is better off in mainline rather
> than kvm.git.
Just to be clear: the final form will need to be wake_up_poll(), not
wake_up_locked_poll. However, I think what Steven did was optimal
because converting to the locked form without coordinating with kvm.git
would break bisectability.
In the irqfd-fixes series, I had 3 patches related to this situation
(1/5 to prepare to change, 2/5 was Davide's patch, and 3/5 did the final
change-over). Now that the vast majority of Davide's work is in
mainline+kvm.git, here is my proposal:
*) drop 2/5 (already upstream, sans the locked POLLHUP)
*) fold 1/5 + 3/5, and add new fs/eventfd.c hunk to convert to locked
variant
*) drop 5/5
Sound good?
-Greg
Avi Kivity wrote:
> On 07/01/2009 10:10 AM, Davide Libenzi wrote:
>> On Wed, 1 Jul 2009, Stephen Rothwell wrote:
>>
>>
>>> Hi Avi,
>>>
>>> Today's linux-next merge of the kvm tree got a conflict in fs/eventfd.c
>>> between commit 133890103b9de08904f909995973e4b5c08a780e ("eventfd:
>>> revised interface and cleanups") from Linus' tree and commit
>>> 28ddf0aebbf546e56efd1951725d5457ce1ebf98 ("eventfd: Allow waiters to be
>>> notified about the eventfd file* going away") from the kvm tree.
>>>
>>> Overlapping changes. I fixed it up (see below), but don't know if this
>>> is the correct fix.
>>>
>>> --
>>> Cheers,
>>> Stephen Rothwell [email protected]
>>>
>>> diff --cc fs/eventfd.c
>>> index 31d12de,72f5f8d..0000000
>>> --- a/fs/eventfd.c
>>> +++ b/fs/eventfd.c
>>> @@@ -105,8 -63,13 +105,13 @@@ static int eventfd_release(struct inod
>>> {
>>> struct eventfd_ctx *ctx = file->private_data;
>>>
>>> - wake_up_poll(&ctx->wqh, POLLHUP);
>>> + /*
>>> + * No need to hold the lock here, since we are on the file
>>> cleanup
>>> + * path and the ones still attached to the wait queue will be
>>> + * serialized by wake_up_locked_poll().
>>> + */
>>> + wake_up_locked_poll(&ctx->wqh, POLLHUP);
>>> - kfree(ctx);
>>> + eventfd_ctx_put(ctx);
>>> return 0;
>>> }
>>>
>>
>> That's fine.
>> There are a couple of extra spaces before the last two -+ in that patch
>> though ;)
>>
>
> No, that's a git N-way diff format. The first column shows the
> changes relative to mainline by kvm.git, and the second the changes to
> kvm.git made by mainline.
>
> I've merged and will push soon, which will resolve the conflict, but I
> think the patch wake_up_locked_poll() is better off in mainline rather
> than kvm.git.
Just to be clear: the final form will need to be wake_up_poll(), not
wake_up_locked_poll. However, I think what Steven did was optimal
because converting to the locked form without coordinating with kvm.git
would break bisectability.
In the irqfd-fixes series, I had 3 patches related to this situation
(1/5 to prepare to change, 2/5 was Davide's patch, and 3/5 did the final
change-over). Now that the vast majority of Davide's work is in
mainline+kvm.git, here is my proposal:
*) drop 2/5 (already upstream, sans the locked POLLHUP)
*) fold 1/5 + 3/5, and add new fs/eventfd.c hunk to convert to locked
variant
*) drop 5/5
Sound good?
-Greg
On Wed, 1 Jul 2009, Gregory Haskins wrote:
> Just to be clear: the final form will need to be wake_up_poll(), not
> wake_up_locked_poll.
Yep, sorry I missed that one.
- Davide