On 2015-08-10 10:57, Damian Hobson-Garcia wrote:
Hi Martin,
Thanks for your comments.
On 2015-08-10 3:39 PM, Martin Sustrik wrote:
quoted
On 2015-08-10 08:23, Damian Hobson-Garcia wrote:
quoted
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:
quoted
From: Martin Sustrik <redacted>
[snip]
quoted
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?
It's a convenience thing. Imagine you are implementing your own file
descriptor type in user space. You create an EFD_MASK socket and a
structure that will hold any state that you need for the socket (tx/rx
25 more quoted lines
My use case was like this:
int s = mysocket();
...
// myrecv() can get the pointer to the structure
// without user having to pass it as an argument
myrecv(s, buf, sizeof(buf));
However, same behaviour can be accomplished by simply keeping
a static array of pointers in the user space.
So let's cut this part out of the patch.
quoted
quoted
[snip]
quoted
@@ -55,6 +69,9 @@ __u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n)
{
+ /* This function should never be used with eventfd in the maskmode. */
+ BUG_ON(ctx->flags & EFD_MASK);
+
...
quoted
@@ -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);
+
...
quoted
@@ -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.
Sure. Feel free to do so.
quoted
[snip]
quoted
@@ -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) {
38 more quoted lines
Keep in mind that the user of your mysocket is not supposed to do
recv() or send() on the raw underlying fd. It's the implementation,
myrecv() and mysend(), that does that.
That being the case and also assuming that we cut the pointer out, there seems
to be little use for recv() any more. The implementation of socket knows
what state it is in and so it doesn't have to retrieve it using recv().
All it has to do is perform whatever business logic is needed and then set new
state of the socket via send().
And the fact there's no clear use case, the logic of recv() is not obvious.
We can very well return ENOTIMPL.
quoted
quoted
quoted
@@ -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.
Sure. No objections.
Thanks for re-submitting the patch!
My pleasure.
quoted
Martin
Damian