Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755124AbbHJGXq (ORCPT ); Mon, 10 Aug 2015 02:23:46 -0400 Received: from mail-pa0-f44.google.com ([209.85.220.44]:33980 "EHLO mail-pa0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755091AbbHJGXk (ORCPT ); Mon, 10 Aug 2015 02:23:40 -0400 Subject: Re: [PATCH] eventfd: implementation of EFD_MASK flag To: sustrik@250bpm.com, viro@zeniv.linux.org.uk References: <1439185906-28180-1-git-send-email-dhobsong@igel.co.jp> <1439185906-28180-2-git-send-email-dhobsong@igel.co.jp> Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, linux-api@vger.kernel.org From: Damian Hobson-Garcia X-Enigmail-Draft-Status: N1110 Message-ID: <55C8436C.1000806@igel.co.jp> Date: Mon, 10 Aug 2015 15:23:40 +0900 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <1439185906-28180-2-git-send-email-dhobsong@igel.co.jp> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3510 Lines: 101 Replying to my own post, but I had the following comments/questions. Martin, if you have any response to my comments I would be very happy to hear them. On 2015-08-10 2:51 PM, Damian Hobson-Garcia wrote: > From: Martin Sustrik > [snip] > > write(2): > > User is allowed to write only buffers containing the following structure: > > struct efd_mask { > __u32 events; > __u64 data; > }; > > The value of 'events' should be any combination of event flags as defined by > poll(2) function (POLLIN, POLLOUT, POLLERR, POLLHUP etc.) Specified events will > be signaled when polling (select, poll, epoll) on the eventfd is done later on. > 'data' is opaque data that are not interpreted by eventfd object. > I'm not fully clear on the purpose that the 'data' member serves. Does this opaque handle need to be tied together with this event synchronization construct? [snip] > @@ -55,6 +69,9 @@ __u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n) > { > + /* This function should never be used with eventfd in the mask mode. */ > + BUG_ON(ctx->flags & EFD_MASK); > + ... > @@ -158,6 +180,9 @@ int eventfd_ctx_remove_wait_queue(struct eventfd_ctx *ctx, wait_queue_t *wait, > { > + /* This function should never be used with eventfd in the mask mode. */ > + BUG_ON(ctx->flags & EFD_MASK); > + ... > @@ -188,6 +213,9 @@ ssize_t eventfd_ctx_read(struct eventfd_ctx *ctx, int no_wait, __u64 *cnt) > + /* This function should never be used with eventfd in the mask mode. */ > + BUG_ON(ctx->flags & EFD_MASK); > + If eventfd_ctx_fileget() returns EINVAL when EFD_MASK is set, I don't think that there will be a way to call these functions in the mask mode, so it should be possible to get rid of the BUG_ON checks. [snip] > @@ -230,6 +258,19 @@ static ssize_t eventfd_read(struct file *file, char __user *buf, size_t count, > ssize_t res; > __u64 cnt; > > + if (ctx->flags & EFD_MASK) { > + struct efd_mask mask; > + > + if (count < sizeof(mask)) > + return -EINVAL; > + spin_lock_irq(&ctx->wqh.lock); > + mask = ctx->mask; > + spin_unlock_irq(&ctx->wqh.lock); > + if (copy_to_user(buf, &mask, sizeof(mask))) > + return -EFAULT; > + return sizeof(mask); > + } > + For the other eventfd modes, reading the value will update the internal state of the eventfd (either clearing or decrementing the counter). Should something similar be done here? I'm thinking of a case where a process is polling on this fd in a loop. Clearing the efd_mask data on read should provide an easy way for the polling process to know if it is seeing new poll events. > @@ -292,8 +351,13 @@ static void eventfd_show_fdinfo(struct seq_file *m, struct file *f) > struct eventfd_ctx *ctx = f->private_data; > > spin_lock_irq(&ctx->wqh.lock); > - seq_printf(m, "eventfd-count: %16llx\n", > - (unsigned long long)ctx->count); > + if (ctx->flags & EFD_MASK) { > + seq_printf(m, "eventfd-mask: %x\n", > + (unsigned)ctx->mask.events); > + } else { > + seq_printf(m, "eventfd-count: %16llx\n", > + (unsigned long long)ctx->count); > + } > spin_unlock_irq(&ctx->wqh.lock); > } I think that putting the EFD_MASK functionality into a different fops structure might be useful for reducing the number of if statements. Thank you, Damian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/