kevent: core files.
Subsystems (8)
Re: [take8 2/2] kevent: poll/select() notifications. Timer notifications.
In reply to Evgeniy PolyakovQuoted 15 lines · On Fri, 11 Aug 2006 12:40:10 +0400 Evgeniy Polyakov <[email protected]> wrote:
poll/select() notifications. Timer notifications.
This patch includes generic poll/select and timer notifications.
kevent_pollworks simialr to epoll and has the same issues (callback is invoked not from internal state machine of the caller, but through process awake).Timer notifications can be used for fine grained per-process time management, since interval timers are very inconvenient to use, and they are limited.
...
kernel/kevent/kevent_poll.c line 211
Quoted 6 lines
kernel/kevent/kevent_poll.c new file +6
-0+static struct lock_class_key kevent_poll_key; + +void kevent_poll_reinit (struct file *file) +{ +lockdep_set_class (&file->st.lock, &kevent_poll_key); +}
Why is this necessary?
Quoted 25 lines
kernel/kevent/kevent_timer.c new file +25
-0+#include <linux/kernel.h> +#include <linux/types.h> +#include <linux/list.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/timer.h> +#include <linux/jiffies.h> +#include <linux/kevent.h> + +static void kevent_timer_func (unsigned long data) +{ +struct kevent *k = (struct kevent *)data; +struct timer_list *t = k->st->origin; + +kevent_storage_ready (k->st,NULL ,KEVENT_MASK_ALL ); +mod_timer (t, jiffies +msecs_to_jiffies (k->event.id.raw[0 ])); +} + +static struct lock_class_key kevent_timer_key; + +static int kevent_timer_enqueue (struct kevent *k) +{ +struct timer_list *t; +struct kevent_storage *st; +int err;
kernel/kevent/kevent_timer.c line 56
Quoted 10 lines
kernel/kevent/kevent_timer.c new file +10
-0+ + t =kmalloc (sizeof (struct timer_list ) +sizeof (struct kevent_storage ), +GFP_KERNEL ); +if (!t) +return -ENOMEM ; + +init_timer (t); + t->function = kevent_timer_func; + t->expires = jiffies +msecs_to_jiffies (k->event.id.raw[0 ]); + t->data = (unsigned long )k;
setup_timer().
kernel/kevent/kevent_timer.c line 58
Quoted 1 line
kernel/kevent/kevent_timer.c new file +1
-0+ st = (struct kevent_storage *)(t+1 );
It would be cleaner to create
struct <something> {
struct timer_list timer;
struct kevent_storage storage;
};kernel/kevent/kevent_timer.c line 62
Quoted 4 lines
kernel/kevent/kevent_timer.c new file +4
-0+ err =kevent_storage_init (t, st); +if (err) + goto err_out_free; +lockdep_set_class (&st->lock, &kevent_timer_key);
Why is this necesary?
Quoted 7 lines
kernel/kevent/kevent_timer.c new file +7
-0+ +kevent_storage_dequeue (st, k); + +kfree (t); + +return 0 ; +}
kernel/kevent/kevent_timer.c line 105
Quoted 10 lines
kernel/kevent/kevent_timer.c new file +10
-0+ +static int kevent_timer_callback (struct kevent *k) +{ +struct kevent_storage *st = k->st; +struct timer_list *t = st->origin; + +if (!t) +return -ENODEV ; + + k->event.ret_data[0 ] = (__u32 )jiffies;
What does this do?
Does it expose jiffies to userspace?
It truncates jiffies on 64-bit machines.
Quoted 1 line
+late_initcall(kevent_init_timer);
module_init() would be more typical. If there was a reason for using late_initcall(), that reason should be commented.
Re: [take8 2/2] kevent: poll/select() notifications. Timer notifications.
Quoted 8 lines · Andrew Morton
kernel/kevent/kevent_poll.c line 211
Quoted 6 lines
kernel/kevent/kevent_poll.c new file +6
-0+static struct lock_class_key kevent_poll_key; + +void kevent_poll_reinit (struct file *file) +{ +lockdep_set_class (&file->st.lock, &kevent_poll_key); +}Why is this necessary?
Locks for all storages are initialized in the same function, so lockdep thinks they are the same, so when later one lock is being held in proces context and other in BH or IRQ lockdep screams, so I reinitialize locks after spin_lock_init().
Quoted 29 lines
Quoted 29 lines
kernel/kevent/kevent_timer.c new file +29
-0+#include <linux/kernel.h> +#include <linux/types.h> +#include <linux/list.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/timer.h> +#include <linux/jiffies.h> +#include <linux/kevent.h> + +static void kevent_timer_func (unsigned long data) +{ +struct kevent *k = (struct kevent *)data; +struct timer_list *t = k->st->origin; + +kevent_storage_ready (k->st,NULL ,KEVENT_MASK_ALL ); +mod_timer (t, jiffies +msecs_to_jiffies (k->event.id.raw[0 ])); +} + +static struct lock_class_key kevent_timer_key; + +static int kevent_timer_enqueue (struct kevent *k) +{ +struct timer_list *t; +struct kevent_storage *st; +int err; + + t =kmalloc (sizeof (struct timer_list ) +sizeof (struct kevent_storage ), +GFP_KERNEL ); +if (!t)
Quoted 8 lines
kernel/kevent/kevent_timer.c line 56
Quoted 6 lines
kernel/kevent/kevent_timer.c new file +6
-0+return -ENOMEM ; + +init_timer (t); + t->function = kevent_timer_func; + t->expires = jiffies +msecs_to_jiffies (k->event.id.raw[0 ]); + t->data = (unsigned long )k;
setup_timer().
I know about it's existens now...
Quoted 9 lines
kernel/kevent/kevent_timer.c line 58
Quoted 1 line
kernel/kevent/kevent_timer.c new file +1
-0+ st = (struct kevent_storage *)(t+1 );It would be cleaner to create
struct <something> { struct timer_list timer; struct kevent_storage storage; };
Quoted 6 lines
kernel/kevent/kevent_timer.c line 62
Quoted 4 lines
kernel/kevent/kevent_timer.c new file +4
-0+ err =kevent_storage_init (t, st); +if (err) + goto err_out_free; +lockdep_set_class (&st->lock, &kevent_timer_key);Why is this necesary?
As I said above kevent_storage_init() initializes locks for all known storages (inode, socket, file and so on), when later those locks are called from different contexts (obviously timer callback can not use the same lock as for example socket one) lockdep screams.
Quoted 20 lines
Quoted 7 lines
kernel/kevent/kevent_timer.c new file +7
-0+ +kevent_storage_dequeue (st, k); + +kfree (t); + +return 0 ; +}kernel/kevent/kevent_timer.c line 105
Quoted 10 lines
kernel/kevent/kevent_timer.c new file +10
-0+ +static int kevent_timer_callback (struct kevent *k) +{ +struct kevent_storage *st = k->st; +struct timer_list *t = st->origin; + +if (!t) +return -ENODEV ; + + k->event.ret_data[0 ] = (__u32 )jiffies;What does this do?
Quoted 3 lines
Does it expose jiffies to userspace?
It truncates jiffies on 64-bit machines.
It is a hint when timer was stopped.
Quoted 4 lines
Quoted 1 line
+late_initcall(kevent_init_timer);
module_init()would be more typical. If there was a reason for usinglate_initcall(), that reason should be commented.
No, there are no reasons to use late_initcall() in any kevent initialization function, I do not use module_init() since kevent can not be modular. It can be replaced with pure __init function. Should it?
Signature · 1 line
Evgeniy Polyakov
Re: [take8 2/2] kevent: poll/select() notifications. Timer notifications.
Quoted 10 lines · On Sat, 12 Aug 2006 12:18:35 +0400 Evgeniy Polyakov <[email protected]> wrote:
Quoted 8 lines · Andrew Morton
kernel/kevent/kevent_poll.c line 211
Quoted 6 lines
kernel/kevent/kevent_poll.c new file +6
-0+static struct lock_class_key kevent_poll_key; + +void kevent_poll_reinit (struct file *file) +{ +lockdep_set_class (&file->st.lock, &kevent_poll_key); +}Why is this necessary?
Quoted 4 lines
Locks for all storages are initialized in the same function, so lockdep thinks they are the same, so when later one lock is being held in proces context and other in BH or IRQ lockdep screams, so I reinitialize locks after
spin_lock_init().
So why not simply run spin_lock_init() in the kevent_storage_init() caller?
Does kevent_poll_reinit() have any callers?
Quoted 8 lines
Quoted 8 lines
kernel/kevent/kevent_timer.c line 58
Quoted 1 line
kernel/kevent/kevent_timer.c new file +1
-0+ st = (struct kevent_storage *)(t+1 );It would be cleaner to create
struct <something> { struct timer_list timer; struct kevent_storage storage; };
You missed this?
Quoted 21 lines
Quoted 21 lines
Quoted 8 lines
kernel/kevent/kevent_timer.c new file +7
-0+ +kevent_storage_dequeue (st, k); + +kfree (t); + +return 0 ; +}kernel/kevent/kevent_timer.c line 105
Quoted 10 lines
kernel/kevent/kevent_timer.c new file +10
-0+ +static int kevent_timer_callback (struct kevent *k) +{ +struct kevent_storage *st = k->st; +struct timer_list *t = st->origin; + +if (!t) +return -ENODEV ; + + k->event.ret_data[0 ] = (__u32 )jiffies;What does this do?
Quoted 5 lines
Quoted 3 lines
Does it expose jiffies to userspace?
It truncates jiffies on 64-bit machines.
It is a hint when timer was stopped.
What does that mean? What is it for?
Does it expose jiffies to userspace?
It truncates jiffies on 64-bit machines.
Please respond to all review comments and questions.
Quoted 9 lines
Quoted 4 lines
Quoted 1 line
+late_initcall(kevent_init_timer);
module_init()would be more typical. If there was a reason for usinglate_initcall(), that reason should be commented.No, there are no reasons to use
late_initcall()in any kevent initialization function, I do not usemodule_init()since kevent can not be modular. It can be replaced with pure__initfunction. Should it?
We use module_init() for non-modular modules all the time. Try doing grep module_init */*.c
Re: [take8 2/2] kevent: poll/select() notifications. Timer notifications.
Quoted 13 lines · Andrew Morton
Quoted 10 lines · On Sat, 12 Aug 2006 12:18:35 +0400 Evgeniy Polyakov <[email protected]> wrote:
Quoted 8 lines · Andrew Morton
kernel/kevent/kevent_poll.c line 211
Quoted 6 lines
kernel/kevent/kevent_poll.c new file +6
-0+static struct lock_class_key kevent_poll_key; + +void kevent_poll_reinit (struct file *file) +{ +lockdep_set_class (&file->st.lock, &kevent_poll_key); +}Why is this necessary?
Quoted 6 lines
Quoted 4 lines
Locks for all storages are initialized in the same function, so lockdep thinks they are the same, so when later one lock is being held in proces context and other in BH or IRQ lockdep screams, so I reinitialize locks after
spin_lock_init().So why not simply run
spin_lock_init()in thekevent_storage_init()caller?
I separated storage initialization into special function, although it is quite simple, but it allows not to have a lot of duplicated steps in each origin (inode, socket, file, AIO, network AIO, poll, timer and so on). If later some members will be changed or added/removed there will be no need to change them all instead change one function.
Quoted 1 line
Does
kevent_poll_reinit()have any callers?
Only poll nitialization.
Quoted 10 lines
Quoted 8 lines
Quoted 8 lines
kernel/kevent/kevent_timer.c line 58
Quoted 1 line
kernel/kevent/kevent_timer.c new file +1
-0+ st = (struct kevent_storage *)(t+1 );It would be cleaner to create
struct <something> { struct timer_list timer; struct kevent_storage storage; };You missed this?
No problem, I will create such structure instead of pointer-based scheme.
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
Quoted 7 lines
kernel/kevent/kevent_timer.c new file +7
-0+ +kevent_storage_dequeue (st, k); + +kfree (t); + +return 0 ; +}kernel/kevent/kevent_timer.c line 105
Quoted 10 lines
kernel/kevent/kevent_timer.c new file +10
-0+ +static int kevent_timer_callback (struct kevent *k) +{ +struct kevent_storage *st = k->st; +struct timer_list *t = st->origin; + +if (!t) +return -ENODEV ; + + k->event.ret_data[0 ] = (__u32 )jiffies;What does this do?
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Does it expose jiffies to userspace?
It truncates jiffies on 64-bit machines.
It is a hint when timer was stopped.
What does that mean? What is it for?
My test code prints it to stdout and I can show a nice graph of how precise timer is.
Quoted 1 line
Does it expose jiffies to userspace?
It is a timestamp of fired condition (measured in jiffies), I can not say if it exposes jiffies to userspace or not.
It is not a requirement to use that field, it can contain zero or any other number, I placed jiffies there.
Quoted 1 line
It truncates jiffies on 64-bit machines.
For the purpose of evnt timestamp it is more than enough.
Quoted 1 line
Please respond to all review comments and questions.
Sorry if something got lost.
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
Quoted 1 line
+late_initcall(kevent_init_timer);
module_init()would be more typical. If there was a reason for usinglate_initcall(), that reason should be commented.
Quoted 7 lines
Quoted 4 lines
No, there are no reasons to use
late_initcall()in any kevent initialization function, I do not usemodule_init()since kevent can not be modular. It can be replaced with pure__initfunction. Should it?We use
module_init()for non-modular modules all the time. Try doing grepmodule_init*/*.c
Ok, I will use it instead late_initcall().
Signature · 1 line
Evgeniy Polyakov
Re: [take8 1/2] kevent: Core files.
In reply to Evgeniy PolyakovReview
Quoted 5 lines · Evgeniy Polyakov
(quoted diff) +5
-0+/* + * Inode events. + */ +#define KEVENT_INODE_CREATE 0x1 +#define KEVENT_INODE_REMOVE 0x2
It would be useful to have gnome/kde notification when hard drives start failing. There was some talk in the past about how to implement that with kobjects. Perhaps you could add for this purpose:
#define KEVENT_BLOCK_CREATE 0x1
#define KEVENT_BLOCK_REMOVE 0x2
#define KEVENT_BLOCK_ERROR 0x4
AFAICT:
The conversation concluded this is the best way to handle ioerrors:
fs/buffer.c +2 -0
@@ -108,6 +108,8 @@ static void buffer_io_error(struct buffeprintk (KERN_ERR "Buffer I/O error on device %s, logical block %Lu\n" ,bdevname (bh->b_bdev, b), (unsigned long long )bh->b_blocknr); + +kevent_block_error (&bh->b_bdev->bd_disk->kobj); }/*
fs/direct-io.c +4 -1
@@ -252,8 +252,11 @@ static void finished_one_bio(struct dio transferred = dio->i_size - offset;/* check for error in completion path */ -if (dio->io_error) +if (dio->io_error) { transferred = dio->io_error; +kevent_block_error ( + &dio->bio->bi_bdev->bd_disk->kobj); + }dio_complete (dio, offset, transferred);
Re: [take8 1/2] kevent: Core files.
Quoted 12 lines · Jeff Carr
Review
Quoted 5 lines · Evgeniy Polyakov
(quoted diff) +5
-0+/* + * Inode events. + */ +#define KEVENT_INODE_CREATE 0x1 +#define KEVENT_INODE_REMOVE 0x2It would be useful to have gnome/kde notification when hard drives start failing. There was some talk in the past about how to implement that with kobjects. Perhaps you could add for this purpose:
Quoted 3 lines
#define
KEVENT_BLOCK_CREATE0x1
#defineKEVENT_BLOCK_REMOVE0x2
#defineKEVENT_BLOCK_ERROR0x4
Do we want it to be on top of kobject (like in your patch), or based on gendisk? I think the latter is better, since kobject reperesents almost any kind of devices, so we will create noticeble overhead for those who do not know about kevents.
Quoted 28 lines
AFAICT:
The conversation concluded this is the best way to handle ioerrors:fs/buffer.c +2
-0@@ -108,6 +108,8 @@ static void buffer_io_error(struct buffeprintk (KERN_ERR "Buffer I/O error on device %s, logical block %Lu\n" ,bdevname (bh->b_bdev, b), (unsigned long long )bh->b_blocknr); + +kevent_block_error (&bh->b_bdev->bd_disk->kobj); }/* fs/direct-io.c +4
-1@@ -252,8 +252,11 @@ static void finished_one_bio(struct dio transferred = dio->i_size - offset;/* check for error in completion path */ -if (dio->io_error) +if (dio->io_error) { transferred = dio->io_error; +kevent_block_error ( + &dio->bio->bi_bdev->bd_disk->kobj); + }dio_complete (dio, offset, transferred);
Signature · 1 line
Evgeniy Polyakov
cover kevent: Generic event handling mechanism.
In reply to Evgeniy PolyakovGeneric event handling mechanism.
Changes from 'take7' patchset:
* new mmap interface (not tested, waiting for other changes to be acked)
- use nopage() method to dynamically substitue pages
- allocate new page for events only when new added kevent requres it
- do not use ugly index dereferencing, use structure instead
- reduced amount of data in the ring (id and flags), maximum 12 pages on x86 per kevent fd
Changes from 'take6' patchset:
* a lot of comments!
* do not use list poisoning for detection of the fact, that entry is in the list
* return number of ready kevents even if copy*user() fails
* strict check for number of kevents in syscall
* use ARRAY_SIZE for array size calculation
* changed superblock magic number
* use SLAB_PANIC instead of direct panic() call
* changed -E* return values
* a lot of small cleanups and indent fixes
Changes from 'take5' patchset:
* removed compilation warnings about unused wariables when lockdep is not turned on
* do not use internal socket structures, use appropriate (exported) wrappers instead
* removed default 1 second timeout
* removed AIO stuff from patchset
Changes from 'take4' patchset:
* use miscdevice instead of chardevice
* comments fixes
Changes from 'take3' patchset:
* removed serializing mutex from kevent_user_wait()
* moved storage list processing to RCU
* removed lockdep screaming - all storage locks are initialized in the same function, so it was learned to differentiate between various cases
* remove kevent from storage if is marked as broken after callback
* fixed a typo in mmaped buffer implementation which would end up in wrong index calcualtion
Changes from 'take2' patchset:
* split kevent_finish_user() to locked and unlocked variants
* do not use KEVENT_STAT ifdefs, use inline functions instead
* use array of callbacks of each type instead of each kevent callback initialization
* changed name of ukevent guarding lock
* use only one kevent lock in kevent_user for all hash buckets instead of per-bucket locks
* do not use kevent_user_ctl structure instead provide needed arguments as syscall parameters
* various indent cleanups
* added optimisation, which is aimed to help when a lot of kevents are being copied from userspace
* mapped buffer (initial) implementation (no userspace yet)
Changes from 'take1' patchset:
- rebased against 2.6.18-git tree
- removed ioctl controlling
- added new syscall kevent_get_events(int fd, unsigned int min_nr, unsigned int max_nr, unsigned int timeout, void __user *buf, unsigned flags)
- use old syscall kevent_ctl for creation/removing, modification and initial kevent initialization
- use mutuxes instead of semaphores
- added file descriptor check and return error if provided descriptor does not match kevent file operations
- various indent fixes
- removed aio_sendfile() declarations.
Thank you.
PATCH 1/2 kevent: Core files.
Core files.
This patch includes core kevent files:
- userspace controlling
- kernelspace interfaces
- initialization
- notification state machines
arch/i386/kernel/syscall_table.S +2 -0
@@ -317,3 +317,5 @@ ENTRY(sys_call_table) .long sys_tee /* 315 */ .long sys_vmsplice .long sys_move_pages + .long sys_kevent_get_events + .long sys_kevent_ctl
arch/x86_64/ia32/ia32entry.S +2 -0
@@ -713,4 +713,6 @@ #endif .quad sys_tee .quad compat_sys_vmsplice .quad compat_sys_move_pages + .quad sys_kevent_get_events + .quad sys_kevent_ctl ia32_syscall_end:
include/asm-i386/unistd.h +3 -1
@@ -323,10 +323,12 @@ #define __NR_sync_file_range 314#define __NR_tee 315#define __NR_vmsplice 316#define __NR_move_pages 317 +#define __NR_kevent_get_events 318 +#define __NR_kevent_ctl 319#ifdef __KERNEL__ -#define NR_syscalls 318 +#define NR_syscalls 320/* * user-visible error numbers are in the range -1 - -128: see
include/asm-x86_64/unistd.h +5 -1
@@ -619,10 +619,14 @@ #define __NR_vmsplice 278 __SYSCALL(__NR_vmsplice, sys_vmsplice)#define __NR_move_pages 279 __SYSCALL(__NR_move_pages, sys_move_pages) +#define __NR_kevent_get_events 280 +__SYSCALL(__NR_kevent_get_events, sys_kevent_get_events) +#define __NR_kevent_ctl 281 +__SYSCALL(__NR_kevent_ctl, sys_kevent_ctl)#ifdef __KERNEL__ -#define __NR_syscall_max __NR_move_pages +#define __NR_syscall_max __NR_kevent_ctl#ifndef __NO_STUBS
include/linux/kevent.h new file +309 -0
309 lines not on this page · raw message
include/linux/kevent_storage.h new file +11 -0
@@ -0,0 +1,11 @@ +#ifndef __KEVENT_STORAGE_H +#define __KEVENT_STORAGE_H + +struct kevent_storage +{ +void *origin;/* Originator's pointer, e.g. struct sock or struct file. Can be NULL. */ +struct list_head list;/* List of queued kevents. */ +spinlock_t lock;/* Protects users queue. */ +}; + +#endif /* __KEVENT_STORAGE_H */
include/linux/syscalls.h +3 -0
@@ -597,4 +597,7 @@ asmlinkage long sys_get_robust_list(intasmlinkage long sys_set_robust_list (struct robust_list_head __user *head,size_t len); +asmlinkage long sys_kevent_get_events (int ctl_fd,unsigned int min,unsigned int max, +unsigned int timeout,void __user *buf,unsigned flags); +asmlinkage long sys_kevent_ctl (int ctl_fd,unsigned int cmd,unsigned int num,void __user *buf);#endif
init/Kconfig +2 -0
@@ -218,6 +218,8 @@ config AUDITSYSCALL such as SELinux. To use audit's filesystem watch feature, please ensure that INOTIFY is configured. +source "kernel/kevent/Kconfig" + config IKCONFIG bool "Kernel .config support" ---help---
kernel/Makefile +1 -0
@@ -47,6 +47,7 @@ obj-$(CONFIG_DETECT_SOFTLOCKUP) += softl obj-$(CONFIG_GENERIC_HARDIRQS) += irq/ obj-$(CONFIG_SECCOMP) += seccomp.o obj-$(CONFIG_RCU_TORTURE_TEST) += rcutorture.o +obj-$(CONFIG_KEVENT) += kevent/ obj-$(CONFIG_RELAY) += relay.o obj-$(CONFIG_TASK_DELAY_ACCT) += delayacct.o obj-$(CONFIG_TASKSTATS) += taskstats.o
kernel/kevent/Kconfig new file +59 -0
59 lines not on this page · raw message
kernel/kevent/Makefile new file +7 -0
@@ -0,0 +1,7 @@ +obj-y := kevent.o kevent_user.o +obj-$(CONFIG_KEVENT_SOCKET) += kevent_socket.o +obj-$(CONFIG_KEVENT_INODE) += kevent_inode.o +obj-$(CONFIG_KEVENT_TIMER) += kevent_timer.o +obj-$(CONFIG_KEVENT_POLL) += kevent_poll.o +obj-$(CONFIG_KEVENT_NAIO) += kevent_naio.o +obj-$(CONFIG_KEVENT_AIO) += kevent_aio.o
kernel/kevent/kevent.c new file +251 -0
251 lines not on this page · raw message
kernel/kevent/kevent_user.c new file +1004 -0
1,004 lines not on this page · raw message
kernel/sys_ni.c +3 -0
@@ -122,6 +122,9 @@ cond_syscall(ppc_rtas);cond_syscall (sys_spu_run);cond_syscall (sys_spu_create); +cond_syscall (sys_kevent_get_events); +cond_syscall (sys_kevent_ctl); +/* mmu depending weak syscall entries */ cond_syscall (sys_mprotect);cond_syscall (sys_msync);
PATCH 2/2 kevent: poll/select() notifications. Timer notifications.
poll/select() notifications. Timer notifications.
This patch includes generic poll/select and timer notifications.
kevent_poll works simialr to epoll and has the same issues (callback is invoked not from internal state machine of the caller, but through process awake).
Timer notifications can be used for fine grained per-process time management, since interval timers are very inconvenient to use, and they are limited.
kernel/kevent/kevent_poll.c new file +220 -0
@@ -0,0 +1,220 @@ +/* +* kevent_poll.c +* +* 2006 Copyright (c) Evgeniy Polyakov <[email protected]> +* All rights reserved. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#include <linux/kernel.h> +#include <linux/types.h> +#include <linux/list.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/timer.h> +#include <linux/file.h> +#include <linux/kevent.h> +#include <linux/poll.h> +#include <linux/fs.h> + +static kmem_cache_t *kevent_poll_container_cache; +static kmem_cache_t *kevent_poll_priv_cache; + +struct kevent_poll_ctl +{ +struct poll_table_struct pt; +struct kevent *k; +}; + +struct kevent_poll_wait_container +{ +struct list_head container_entry; +wait_queue_head_t *whead; +wait_queue_t wait; +struct kevent *k; +}; + +struct kevent_poll_private +{ +struct list_head container_list; +spinlock_t container_lock; +}; + +static int kevent_poll_enqueue (struct kevent *k); +static int kevent_poll_dequeue (struct kevent *k); +static int kevent_poll_callback (struct kevent *k); + +static int kevent_poll_wait_callback (wait_queue_t *wait, +unsigned mode,int sync,void *key) +{ +struct kevent_poll_wait_container *cont = + container_of(wait, struct kevent_poll_wait_container, wait); +struct kevent *k = cont->k; +struct file *file = k->st->origin; +u32 revents; + + revents = file->f_op->poll (file,NULL ); + +kevent_storage_ready (k->st,NULL , revents); + +return 0 ; +} + +static void kevent_poll_qproc (struct file *file,wait_queue_head_t *whead, +struct poll_table_struct *poll_table) +{ +struct kevent *k = + container_of(poll_table, struct kevent_poll_ctl, pt)->k; +struct kevent_poll_private *priv = k->priv; +struct kevent_poll_wait_container *cont; +unsigned long flags; + + cont =kmem_cache_alloc (kevent_poll_container_cache,SLAB_KERNEL ); +if (!cont) { +kevent_break (k); +return ; + } + + cont->k = k; +init_waitqueue_func_entry (&cont->wait, kevent_poll_wait_callback); + cont->whead = whead; + +spin_lock_irqsave (&priv->container_lock, flags); +list_add_tail (&cont->container_entry, &priv->container_list); +spin_unlock_irqrestore (&priv->container_lock, flags); + +add_wait_queue (whead, &cont->wait); +} + +static int kevent_poll_enqueue (struct kevent *k) +{ +struct file *file; +int err, ready =0 ; +unsigned int revents; +struct kevent_poll_ctl ctl; +struct kevent_poll_private *priv; + + file =fget (k->event.id.raw[0 ]); +if (!file) +return -ENODEV ; + + err = -EINVAL ; +if (!file->f_op || !file->f_op->poll) + goto err_out_fput; + + err = -ENOMEM ; + priv =kmem_cache_alloc (kevent_poll_priv_cache,SLAB_KERNEL ); +if (!priv) + goto err_out_fput; + +spin_lock_init (&priv->container_lock); +INIT_LIST_HEAD (&priv->container_list); + + k->priv = priv; + + ctl.k = k; +init_poll_funcptr (&ctl.pt, &kevent_poll_qproc); + + err =kevent_storage_enqueue (&file->st, k); +if (err) + goto err_out_free; + + revents = file->f_op->poll (file, &ctl.pt); +if (revents & k->event.event) { + ready =1 ; +kevent_poll_dequeue (k); + } + +return ready; + +err_out_free: +kmem_cache_free (kevent_poll_priv_cache, priv); +err_out_fput: +fput (file); +return err; +} + +static int kevent_poll_dequeue (struct kevent *k) +{ +struct file *file = k->st->origin; +struct kevent_poll_private *priv = k->priv; +struct kevent_poll_wait_container *w, *n; +unsigned long flags; + +kevent_storage_dequeue (k->st, k); + +spin_lock_irqsave (&priv->container_lock, flags); +list_for_each_entry_safe (w, n, &priv->container_list, container_entry) { +list_del (&w->container_entry); +remove_wait_queue (w->whead, &w->wait); +kmem_cache_free (kevent_poll_container_cache, w); + } +spin_unlock_irqrestore (&priv->container_lock, flags); + +kmem_cache_free (kevent_poll_priv_cache, priv); + k->priv =NULL ; + +fput (file); + +return 0 ; +} + +static int kevent_poll_callback (struct kevent *k) +{ +struct file *file = k->st->origin; +unsigned int revents = file->f_op->poll (file,NULL ); +return (revents & k->event.event); +} + +static int __init kevent_poll_sys_init (void ) +{ +struct kevent_callbacks *pc = &kevent_registered_callbacks[KEVENT_POLL ]; + + kevent_poll_container_cache =kmem_cache_create ("kevent_poll_container_cache" , +sizeof (struct kevent_poll_wait_container ),0 ,0 ,NULL ,NULL ); +if (!kevent_poll_container_cache) { +printk (KERN_ERR "Failed to create kevent poll container cache.\n" ); +return -ENOMEM ; + } + + kevent_poll_priv_cache =kmem_cache_create ("kevent_poll_priv_cache" , +sizeof (struct kevent_poll_private ),0 ,0 ,NULL ,NULL ); +if (!kevent_poll_priv_cache) { +printk (KERN_ERR "Failed to create kevent poll private data cache.\n" ); +kmem_cache_destroy (kevent_poll_container_cache); + kevent_poll_container_cache =NULL ; +return -ENOMEM ; + } + + pc->enqueue = &kevent_poll_enqueue; + pc->dequeue = &kevent_poll_dequeue; + pc->callback = &kevent_poll_callback; + +printk (KERN_INFO "Kevent poll()/select() subsystem has been initialized.\n" ); +return 0 ; +} + +static struct lock_class_key kevent_poll_key; + +void kevent_poll_reinit (struct file *file) +{ +lockdep_set_class (&file->st.lock, &kevent_poll_key); +} + +static void __exit kevent_poll_sys_fini(void) +{ +kmem_cache_destroy (kevent_poll_priv_cache); +kmem_cache_destroy (kevent_poll_container_cache); +} + +module_init (kevent_poll_sys_init); +module_exit (kevent_poll_sys_fini);
kernel/kevent/kevent_timer.c new file +119 -0
119 lines not on this page · raw message
cover kevent: Generic event handling mechanism.
In reply to Evgeniy PolyakovGeneric event handling mechanism.
Changes from 'take8' patchset:
* fixed mmap release bug
* use module_init() instead of late_initcall()
* use better structures for timer notifications
Changes from 'take7' patchset:
* new mmap interface (not tested, waiting for other changes to be acked)
- use nopage() method to dynamically substitue pages
- allocate new page for events only when new added kevent requres it
- do not use ugly index dereferencing, use structure instead
- reduced amount of data in the ring (id and flags), maximum 12 pages on x86 per kevent fd
Changes from 'take6' patchset:
* a lot of comments!
* do not use list poisoning for detection of the fact, that entry is in the list
* return number of ready kevents even if copy*user() fails
* strict check for number of kevents in syscall
* use ARRAY_SIZE for array size calculation
* changed superblock magic number
* use SLAB_PANIC instead of direct panic() call
* changed -E* return values
* a lot of small cleanups and indent fixes
Changes from 'take5' patchset:
* removed compilation warnings about unused wariables when lockdep is not turned on
* do not use internal socket structures, use appropriate (exported) wrappers instead
* removed default 1 second timeout
* removed AIO stuff from patchset
Changes from 'take4' patchset:
* use miscdevice instead of chardevice
* comments fixes
Changes from 'take3' patchset:
* removed serializing mutex from kevent_user_wait()
* moved storage list processing to RCU
* removed lockdep screaming - all storage locks are initialized in the same function, so it was learned to differentiate between various cases
* remove kevent from storage if is marked as broken after callback
* fixed a typo in mmaped buffer implementation which would end up in wrong index calcualtion
Changes from 'take2' patchset:
* split kevent_finish_user() to locked and unlocked variants
* do not use KEVENT_STAT ifdefs, use inline functions instead
* use array of callbacks of each type instead of each kevent callback initialization
* changed name of ukevent guarding lock
* use only one kevent lock in kevent_user for all hash buckets instead of per-bucket locks
* do not use kevent_user_ctl structure instead provide needed arguments as syscall parameters
* various indent cleanups
* added optimisation, which is aimed to help when a lot of kevents are being copied from userspace
* mapped buffer (initial) implementation (no userspace yet)
Changes from 'take1' patchset:
- rebased against 2.6.18-git tree
- removed ioctl controlling
- added new syscall kevent_get_events(int fd, unsigned int min_nr, unsigned int max_nr, unsigned int timeout, void __user *buf, unsigned flags)
- use old syscall kevent_ctl for creation/removing, modification and initial kevent initialization
- use mutuxes instead of semaphores
- added file descriptor check and return error if provided descriptor does not match kevent file operations
- various indent fixes
- removed aio_sendfile() declarations.
Thank you.
PATCH 1/2 kevent: Core files.
Core files.
This patch includes core kevent files:
- userspace controlling
- kernelspace interfaces
- initialization
- notification state machines
arch/i386/kernel/syscall_table.S +2 -0
@@ -317,3 +317,5 @@ ENTRY(sys_call_table) .long sys_tee /* 315 */ .long sys_vmsplice .long sys_move_pages + .long sys_kevent_get_events + .long sys_kevent_ctl
arch/x86_64/ia32/ia32entry.S +2 -0
@@ -713,4 +713,6 @@ #endif .quad sys_tee .quad compat_sys_vmsplice .quad compat_sys_move_pages + .quad sys_kevent_get_events + .quad sys_kevent_ctl ia32_syscall_end:
include/asm-i386/unistd.h +3 -1
@@ -323,10 +323,12 @@ #define __NR_sync_file_range 314#define __NR_tee 315#define __NR_vmsplice 316#define __NR_move_pages 317 +#define __NR_kevent_get_events 318 +#define __NR_kevent_ctl 319#ifdef __KERNEL__ -#define NR_syscalls 318 +#define NR_syscalls 320/* * user-visible error numbers are in the range -1 - -128: see
include/asm-x86_64/unistd.h +5 -1
@@ -619,10 +619,14 @@ #define __NR_vmsplice 278 __SYSCALL(__NR_vmsplice, sys_vmsplice)#define __NR_move_pages 279 __SYSCALL(__NR_move_pages, sys_move_pages) +#define __NR_kevent_get_events 280 +__SYSCALL(__NR_kevent_get_events, sys_kevent_get_events) +#define __NR_kevent_ctl 281 +__SYSCALL(__NR_kevent_ctl, sys_kevent_ctl)#ifdef __KERNEL__ -#define __NR_syscall_max __NR_move_pages +#define __NR_syscall_max __NR_kevent_ctl#ifndef __NO_STUBS
include/linux/kevent.h new file +310 -0
310 lines not on this page · raw message
include/linux/kevent_storage.h new file +11 -0
@@ -0,0 +1,11 @@ +#ifndef __KEVENT_STORAGE_H +#define __KEVENT_STORAGE_H + +struct kevent_storage +{ +void *origin;/* Originator's pointer, e.g. struct sock or struct file. Can be NULL. */ +struct list_head list;/* List of queued kevents. */ +spinlock_t lock;/* Protects users queue. */ +}; + +#endif /* __KEVENT_STORAGE_H */
include/linux/syscalls.h +3 -0
@@ -597,4 +597,7 @@ asmlinkage long sys_get_robust_list(intasmlinkage long sys_set_robust_list (struct robust_list_head __user *head,size_t len); +asmlinkage long sys_kevent_get_events (int ctl_fd,unsigned int min,unsigned int max, +unsigned int timeout,void __user *buf,unsigned flags); +asmlinkage long sys_kevent_ctl (int ctl_fd,unsigned int cmd,unsigned int num,void __user *buf);#endif
init/Kconfig +2 -0
@@ -218,6 +218,8 @@ config AUDITSYSCALL such as SELinux. To use audit's filesystem watch feature, please ensure that INOTIFY is configured. +source "kernel/kevent/Kconfig" + config IKCONFIG bool "Kernel .config support" ---help---
kernel/Makefile +1 -0
@@ -47,6 +47,7 @@ obj-$(CONFIG_DETECT_SOFTLOCKUP) += softl obj-$(CONFIG_GENERIC_HARDIRQS) += irq/ obj-$(CONFIG_SECCOMP) += seccomp.o obj-$(CONFIG_RCU_TORTURE_TEST) += rcutorture.o +obj-$(CONFIG_KEVENT) += kevent/ obj-$(CONFIG_RELAY) += relay.o obj-$(CONFIG_TASK_DELAY_ACCT) += delayacct.o obj-$(CONFIG_TASKSTATS) += taskstats.o
kernel/kevent/Kconfig new file +59 -0
59 lines not on this page · raw message
kernel/kevent/Makefile new file +7 -0
@@ -0,0 +1,7 @@ +obj-y := kevent.o kevent_user.o +obj-$(CONFIG_KEVENT_SOCKET) += kevent_socket.o +obj-$(CONFIG_KEVENT_INODE) += kevent_inode.o +obj-$(CONFIG_KEVENT_TIMER) += kevent_timer.o +obj-$(CONFIG_KEVENT_POLL) += kevent_poll.o +obj-$(CONFIG_KEVENT_NAIO) += kevent_naio.o +obj-$(CONFIG_KEVENT_AIO) += kevent_aio.o
kernel/kevent/kevent.c new file +249 -0
249 lines not on this page · raw message
kernel/kevent/kevent_user.c new file +1007 -0
1,007 lines not on this page · raw message
kernel/sys_ni.c +3 -0
@@ -122,6 +122,9 @@ cond_syscall(ppc_rtas);cond_syscall (sys_spu_run);cond_syscall (sys_spu_create); +cond_syscall (sys_kevent_get_events); +cond_syscall (sys_kevent_ctl); +/* mmu depending weak syscall entries */ cond_syscall (sys_mprotect);cond_syscall (sys_msync);
PATCH 2/2 kevent: poll/select() notifications. Timer notifications.
poll/select() notifications. Timer notifications.
This patch includes generic poll/select and timer notifications.
kevent_poll works simialr to epoll and has the same issues (callback is invoked not from internal state machine of the caller, but through process awake).
Timer notifications can be used for fine grained per-process time management, since interval timers are very inconvenient to use, and they are limited.
kernel/kevent/kevent_poll.c new file +220 -0
@@ -0,0 +1,220 @@ +/* +* kevent_poll.c +* +* 2006 Copyright (c) Evgeniy Polyakov <[email protected]> +* All rights reserved. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#include <linux/kernel.h> +#include <linux/types.h> +#include <linux/list.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/timer.h> +#include <linux/file.h> +#include <linux/kevent.h> +#include <linux/poll.h> +#include <linux/fs.h> + +static kmem_cache_t *kevent_poll_container_cache; +static kmem_cache_t *kevent_poll_priv_cache; + +struct kevent_poll_ctl +{ +struct poll_table_struct pt; +struct kevent *k; +}; + +struct kevent_poll_wait_container +{ +struct list_head container_entry; +wait_queue_head_t *whead; +wait_queue_t wait; +struct kevent *k; +}; + +struct kevent_poll_private +{ +struct list_head container_list; +spinlock_t container_lock; +}; + +static int kevent_poll_enqueue (struct kevent *k); +static int kevent_poll_dequeue (struct kevent *k); +static int kevent_poll_callback (struct kevent *k); + +static int kevent_poll_wait_callback (wait_queue_t *wait, +unsigned mode,int sync,void *key) +{ +struct kevent_poll_wait_container *cont = + container_of(wait, struct kevent_poll_wait_container, wait); +struct kevent *k = cont->k; +struct file *file = k->st->origin; +u32 revents; + + revents = file->f_op->poll (file,NULL ); + +kevent_storage_ready (k->st,NULL , revents); + +return 0 ; +} + +static void kevent_poll_qproc (struct file *file,wait_queue_head_t *whead, +struct poll_table_struct *poll_table) +{ +struct kevent *k = + container_of(poll_table, struct kevent_poll_ctl, pt)->k; +struct kevent_poll_private *priv = k->priv; +struct kevent_poll_wait_container *cont; +unsigned long flags; + + cont =kmem_cache_alloc (kevent_poll_container_cache,SLAB_KERNEL ); +if (!cont) { +kevent_break (k); +return ; + } + + cont->k = k; +init_waitqueue_func_entry (&cont->wait, kevent_poll_wait_callback); + cont->whead = whead; + +spin_lock_irqsave (&priv->container_lock, flags); +list_add_tail (&cont->container_entry, &priv->container_list); +spin_unlock_irqrestore (&priv->container_lock, flags); + +add_wait_queue (whead, &cont->wait); +} + +static int kevent_poll_enqueue (struct kevent *k) +{ +struct file *file; +int err, ready =0 ; +unsigned int revents; +struct kevent_poll_ctl ctl; +struct kevent_poll_private *priv; + + file =fget (k->event.id.raw[0 ]); +if (!file) +return -ENODEV ; + + err = -EINVAL ; +if (!file->f_op || !file->f_op->poll) + goto err_out_fput; + + err = -ENOMEM ; + priv =kmem_cache_alloc (kevent_poll_priv_cache,SLAB_KERNEL ); +if (!priv) + goto err_out_fput; + +spin_lock_init (&priv->container_lock); +INIT_LIST_HEAD (&priv->container_list); + + k->priv = priv; + + ctl.k = k; +init_poll_funcptr (&ctl.pt, &kevent_poll_qproc); + + err =kevent_storage_enqueue (&file->st, k); +if (err) + goto err_out_free; + + revents = file->f_op->poll (file, &ctl.pt); +if (revents & k->event.event) { + ready =1 ; +kevent_poll_dequeue (k); + } + +return ready; + +err_out_free: +kmem_cache_free (kevent_poll_priv_cache, priv); +err_out_fput: +fput (file); +return err; +} + +static int kevent_poll_dequeue (struct kevent *k) +{ +struct file *file = k->st->origin; +struct kevent_poll_private *priv = k->priv; +struct kevent_poll_wait_container *w, *n; +unsigned long flags; + +kevent_storage_dequeue (k->st, k); + +spin_lock_irqsave (&priv->container_lock, flags); +list_for_each_entry_safe (w, n, &priv->container_list, container_entry) { +list_del (&w->container_entry); +remove_wait_queue (w->whead, &w->wait); +kmem_cache_free (kevent_poll_container_cache, w); + } +spin_unlock_irqrestore (&priv->container_lock, flags); + +kmem_cache_free (kevent_poll_priv_cache, priv); + k->priv =NULL ; + +fput (file); + +return 0 ; +} + +static int kevent_poll_callback (struct kevent *k) +{ +struct file *file = k->st->origin; +unsigned int revents = file->f_op->poll (file,NULL ); +return (revents & k->event.event); +} + +static int __init kevent_poll_sys_init (void ) +{ +struct kevent_callbacks *pc = &kevent_registered_callbacks[KEVENT_POLL ]; + + kevent_poll_container_cache =kmem_cache_create ("kevent_poll_container_cache" , +sizeof (struct kevent_poll_wait_container ),0 ,0 ,NULL ,NULL ); +if (!kevent_poll_container_cache) { +printk (KERN_ERR "Failed to create kevent poll container cache.\n" ); +return -ENOMEM ; + } + + kevent_poll_priv_cache =kmem_cache_create ("kevent_poll_priv_cache" , +sizeof (struct kevent_poll_private ),0 ,0 ,NULL ,NULL ); +if (!kevent_poll_priv_cache) { +printk (KERN_ERR "Failed to create kevent poll private data cache.\n" ); +kmem_cache_destroy (kevent_poll_container_cache); + kevent_poll_container_cache =NULL ; +return -ENOMEM ; + } + + pc->enqueue = &kevent_poll_enqueue; + pc->dequeue = &kevent_poll_dequeue; + pc->callback = &kevent_poll_callback; + +printk (KERN_INFO "Kevent poll()/select() subsystem has been initialized.\n" ); +return 0 ; +} + +static struct lock_class_key kevent_poll_key; + +void kevent_poll_reinit (struct file *file) +{ +lockdep_set_class (&file->st.lock, &kevent_poll_key); +} + +static void __exit kevent_poll_sys_fini(void) +{ +kmem_cache_destroy (kevent_poll_priv_cache); +kmem_cache_destroy (kevent_poll_container_cache); +} + +module_init (kevent_poll_sys_init); +module_exit (kevent_poll_sys_fini);
kernel/kevent/kevent_timer.c new file +108 -0
108 lines not on this page · raw message
Re: [take9 2/2] kevent: poll/select() notifications. Timer notifications.
Quoted 8 lines · Evgeniy Polyakov
poll/select() notifications. Timer notifications.
This patch includes generic poll/select and timer notifications.
kevent_pollworks simialr to epoll and has the same issues (callback is invoked not from internal state machine of the caller, but through process awake).
I'm not a big fan of duplicating code over and over. kevent is a candidate for a generic event devlivery mechanisms which is a _very_ good thing. But starting that system by duplicating existing functionality is not very nice.
What speaks against a patch the recplaces the epoll core by something that build on kevent while still supporting the epoll interface as a compatibility shim?
Quoted 3 lines
Timer notifications can be used for fine grained per-process time management, since interval timers are very inconvenient to use, and they are limited.
I have similar reservations about this one. Having timers as part of a generic events system is very nice, but having so much duplicated functionality is not. Cc'ed Thomas on behalf of the Timer cabal if there's a point in integrating this into a larger framework of timer code.
Also it would be nice if you could submit each of the notifications as a patch on it's own.
Quoted 342 lines
kernel/kevent/kevent_poll.c new file +220
-0@@ -0,0 +1,220 @@ +/* +* kevent_poll.c +* +* 2006 Copyright (c) Evgeniy Polyakov <[email protected]> +* All rights reserved. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#include <linux/kernel.h> +#include <linux/types.h> +#include <linux/list.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/timer.h> +#include <linux/file.h> +#include <linux/kevent.h> +#include <linux/poll.h> +#include <linux/fs.h> + +static kmem_cache_t *kevent_poll_container_cache; +static kmem_cache_t *kevent_poll_priv_cache; + +struct kevent_poll_ctl +{ +struct poll_table_struct pt; +struct kevent *k; +}; + +struct kevent_poll_wait_container +{ +struct list_head container_entry; +wait_queue_head_t *whead; +wait_queue_t wait; +struct kevent *k; +}; + +struct kevent_poll_private +{ +struct list_head container_list; +spinlock_t container_lock; +}; + +static int kevent_poll_enqueue (struct kevent *k); +static int kevent_poll_dequeue (struct kevent *k); +static int kevent_poll_callback (struct kevent *k); + +static int kevent_poll_wait_callback (wait_queue_t *wait, +unsigned mode,int sync,void *key) +{ +struct kevent_poll_wait_container *cont = + container_of(wait, struct kevent_poll_wait_container, wait); +struct kevent *k = cont->k; +struct file *file = k->st->origin; +u32 revents; + + revents = file->f_op->poll (file,NULL ); + +kevent_storage_ready (k->st,NULL , revents); + +return 0 ; +} + +static void kevent_poll_qproc (struct file *file,wait_queue_head_t *whead, +struct poll_table_struct *poll_table) +{ +struct kevent *k = + container_of(poll_table, struct kevent_poll_ctl, pt)->k; +struct kevent_poll_private *priv = k->priv; +struct kevent_poll_wait_container *cont; +unsigned long flags; + + cont =kmem_cache_alloc (kevent_poll_container_cache,SLAB_KERNEL ); +if (!cont) { +kevent_break (k); +return ; + } + + cont->k = k; +init_waitqueue_func_entry (&cont->wait, kevent_poll_wait_callback); + cont->whead = whead; + +spin_lock_irqsave (&priv->container_lock, flags); +list_add_tail (&cont->container_entry, &priv->container_list); +spin_unlock_irqrestore (&priv->container_lock, flags); + +add_wait_queue (whead, &cont->wait); +} + +static int kevent_poll_enqueue (struct kevent *k) +{ +struct file *file; +int err, ready =0 ; +unsigned int revents; +struct kevent_poll_ctl ctl; +struct kevent_poll_private *priv; + + file =fget (k->event.id.raw[0 ]); +if (!file) +return -ENODEV ; + + err = -EINVAL ; +if (!file->f_op || !file->f_op->poll) + goto err_out_fput; + + err = -ENOMEM ; + priv =kmem_cache_alloc (kevent_poll_priv_cache,SLAB_KERNEL ); +if (!priv) + goto err_out_fput; + +spin_lock_init (&priv->container_lock); +INIT_LIST_HEAD (&priv->container_list); + + k->priv = priv; + + ctl.k = k; +init_poll_funcptr (&ctl.pt, &kevent_poll_qproc); + + err =kevent_storage_enqueue (&file->st, k); +if (err) + goto err_out_free; + + revents = file->f_op->poll (file, &ctl.pt); +if (revents & k->event.event) { + ready =1 ; +kevent_poll_dequeue (k); + } + +return ready; + +err_out_free: +kmem_cache_free (kevent_poll_priv_cache, priv); +err_out_fput: +fput (file); +return err; +} + +static int kevent_poll_dequeue (struct kevent *k) +{ +struct file *file = k->st->origin; +struct kevent_poll_private *priv = k->priv; +struct kevent_poll_wait_container *w, *n; +unsigned long flags; + +kevent_storage_dequeue (k->st, k); + +spin_lock_irqsave (&priv->container_lock, flags); +list_for_each_entry_safe (w, n, &priv->container_list, container_entry) { +list_del (&w->container_entry); +remove_wait_queue (w->whead, &w->wait); +kmem_cache_free (kevent_poll_container_cache, w); + } +spin_unlock_irqrestore (&priv->container_lock, flags); + +kmem_cache_free (kevent_poll_priv_cache, priv); + k->priv =NULL ; + +fput (file); + +return 0 ; +} + +static int kevent_poll_callback (struct kevent *k) +{ +struct file *file = k->st->origin; +unsigned int revents = file->f_op->poll (file,NULL ); +return (revents & k->event.event); +} + +static int __init kevent_poll_sys_init (void ) +{ +struct kevent_callbacks *pc = &kevent_registered_callbacks[KEVENT_POLL ]; + + kevent_poll_container_cache =kmem_cache_create ("kevent_poll_container_cache" , +sizeof (struct kevent_poll_wait_container ),0 ,0 ,NULL ,NULL ); +if (!kevent_poll_container_cache) { +printk (KERN_ERR "Failed to create kevent poll container cache.\n" ); +return -ENOMEM ; + } + + kevent_poll_priv_cache =kmem_cache_create ("kevent_poll_priv_cache" , +sizeof (struct kevent_poll_private ),0 ,0 ,NULL ,NULL ); +if (!kevent_poll_priv_cache) { +printk (KERN_ERR "Failed to create kevent poll private data cache.\n" ); +kmem_cache_destroy (kevent_poll_container_cache); + kevent_poll_container_cache =NULL ; +return -ENOMEM ; + } + + pc->enqueue = &kevent_poll_enqueue; + pc->dequeue = &kevent_poll_dequeue; + pc->callback = &kevent_poll_callback; + +printk (KERN_INFO "Kevent poll()/select() subsystem has been initialized.\n" ); +return 0 ; +} + +static struct lock_class_key kevent_poll_key; + +void kevent_poll_reinit (struct file *file) +{ +lockdep_set_class (&file->st.lock, &kevent_poll_key); +} + +static void __exit kevent_poll_sys_fini(void) +{ +kmem_cache_destroy (kevent_poll_priv_cache); +kmem_cache_destroy (kevent_poll_container_cache); +} + +module_init (kevent_poll_sys_init); +module_exit (kevent_poll_sys_fini);kernel/kevent/kevent_timer.c new file +108
-1@@ -0,0 +1,108 @@ +/* +* kevent_timer.c +* +* 2006 Copyright (c) Evgeniy Polyakov <[email protected]> +* All rights reserved. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA +*/ + +#include <linux/kernel.h> +#include <linux/types.h> +#include <linux/list.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/timer.h> +#include <linux/jiffies.h> +#include <linux/kevent.h> + +struct kevent_timer +{ +struct timer_list ktimer; +struct kevent_storage ktimer_storage; +}; + +static void kevent_timer_func (unsigned long data) +{ +struct kevent *k = (struct kevent *)data; +struct timer_list *t = k->st->origin; + +kevent_storage_ready (k->st,NULL ,KEVENT_MASK_ALL ); +mod_timer (t, jiffies +msecs_to_jiffies (k->event.id.raw[0 ])); +} + +static struct lock_class_key kevent_timer_key; + +static int kevent_timer_enqueue (struct kevent *k) +{ +int err; +struct kevent_timer *t; + + t =kmalloc (sizeof (struct kevent_timer ),GFP_KERNEL ); +if (!t) +return -ENOMEM ; + +setup_timer (&t->ktimer, &kevent_timer_func, (unsigned long )k); + + err =kevent_storage_init (&t->ktimer, &t->ktimer_storage); +if (err) + goto err_out_free; +lockdep_set_class (&t->ktimer_storage.lock, &kevent_timer_key); + + err =kevent_storage_enqueue (&t->ktimer_storage, k); +if (err) + goto err_out_st_fini; + +mod_timer (&t->ktimer, jiffies +msecs_to_jiffies (k->event.id.raw[0 ])); + +return 0 ; + +err_out_st_fini: +kevent_storage_fini (&t->ktimer_storage); +err_out_free: +kfree (t); + +return err; +} + +static int kevent_timer_dequeue (struct kevent *k) +{ +struct kevent_storage *st = k->st; + struct kevent_timer *t = container_of(st, struct kevent_timer, ktimer_storage); + +del_timer_sync (&t->ktimer); +kevent_storage_dequeue (st, k); +kfree (t); + +return 0 ; +} + +static int kevent_timer_callback (struct kevent *k) +{ + k->event.ret_data[0 ] = (__u32 )jiffies; +return 1 ; +} + +static int __init kevent_init_timer (void ) +{ +struct kevent_callbacks *tc = &kevent_registered_callbacks[KEVENT_TIMER ]; + + tc->enqueue = &kevent_timer_enqueue; + tc->dequeue = &kevent_timer_dequeue; + tc->callback = &kevent_timer_callback; + +return 0 ; +} +module_init (kevent_init_timer); -
Quoted 3 lines
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
---end quoted text---
Re: [take9 2/2] kevent: poll/select() notifications. Timer notifications.
Quoted 10 lines · Christoph Hellwig
Quoted 8 lines · Evgeniy Polyakov
poll/select() notifications. Timer notifications.
This patch includes generic poll/select and timer notifications.
kevent_pollworks simialr to epoll and has the same issues (callback is invoked not from internal state machine of the caller, but through process awake).
Quoted 7 lines
I'm not a big fan of duplicating code over and over. kevent is a candidate for a generic event devlivery mechanisms which is a _very_ good thing. But starting that system by duplicating existing functionality is not very nice.
What speaks against a patch the recplaces the epoll core by something that build on kevent while still supporting the epoll interface as a compatibility shim?
There is no problem from my side, but epoll and kevent_poll differs on some aspects, so it can be better to not replace them for a while.
Quoted 4 lines
Quoted 3 lines
Timer notifications can be used for fine grained per-process time management, since interval timers are very inconvenient to use, and they are limited.
Quoted 8 lines
I have similar reservations about this one. Having timers as part of a generic events system is very nice, but having so much duplicated functionality is not. Cc'ed Thomas on behalf of the Timer cabal if there's a point in integrating this into a larger framework of timer code.
Also it would be nice if you could submit each of the notifications as a patch on it's own.
Ok.
Signature · 1 line
Evgeniy Polyakov
Re: [take9 2/2] kevent: poll/select() notifications. Timer notifications.
Quoted 6 lines · Evgeniy Polyakov
Quoted 3 lines
What speaks against a patch the recplaces the epoll core by something that build on kevent while still supporting the epoll interface as a compatibility shim?
There is no problem from my side, but epoll and
kevent_polldiffers on some aspects, so it can be better to not replace them for a while.
Please explain the differences and why they are important. We really shouldn't keep on adding code without beeing able to replace older bits. If there's a really good reason we can keep things separate, but
"epoll and kevent_poll differs on some aspects"
is not one :)
Re: [take9 2/2] kevent: poll/select() notifications. Timer notifications.
Quoted 8 lines · Christoph Hellwig
Quoted 6 lines · Evgeniy Polyakov
Quoted 3 lines
What speaks against a patch the recplaces the epoll core by something that build on kevent while still supporting the epoll interface as a compatibility shim?
There is no problem from my side, but epoll and
kevent_polldiffers on some aspects, so it can be better to not replace them for a while.
Quoted 7 lines
Please explain the differences and why they are important. We really shouldn't keep on adding code without beeing able to replace older bits. If there's a really good reason we can keep things separate, but
"epoll and
kevent_polldiffers on some aspects"is not one :)
kevent_poll uses hash table (actually it is kevent that uses table), locking is simpler and part of it is hidden in kevent core. Actually kevent_poll is just a container allocator for poll wait queue. So epoll does not differ (except hash/tree and locking, which is based on locks for pathes which are shared in kevent with those ones which can be called from irq/bh context) from kevent + kevent_poll. And since kevent_poll can be not selected while epoll is always there (until embedded config is turned on), I recommend to have them both. Or always turn kevent on :)
Signature · 1 line
Evgeniy Polyakov
Re: [take9 2/2] kevent: poll/select() notifications. Timer notifications.
Quoted 6 lines · Evgeniy Polyakov
Quoted 5 lines
If there's a really good reason we can keep things separate, but
"epoll and
kevent_polldiffers on some aspects"is not one :)
Quoted 9 lines
kevent_polluses hash table (actually it is kevent that uses table), locking is simpler and part of it is hidden in kevent core. Actuallykevent_pollis just a container allocator for poll wait queue. So epoll does not differ (except hash/tree and locking, which is based on locks for pathes which are shared in kevent with those ones which can be called from irq/bh context) from kevent +kevent_poll. And sincekevent_pollcan be not selected while epoll is always there (until embedded config is turned on), I recommend to have them both. Or always turn kevent on :)
You mention a lot of implementation details that absoultely shouldn't matter to the userspace interface.
I might not have explained enough what the point behind all this is, so I'll try to explain it again:
- the fate of aio, inotify, epoll, etc shows we badly need a generic event mechnism that unifies event based interfaces of various subsystem. Only having a single mechanisms allows things like unified event loops and gives application progreammers the chance to learn that one interface for real and get it right.
- kevent looks like the right way to do this. but to show it can really archive this it needs to show it can do the things the existing event systems can do at least as good. reimplementing their user interfaces ontop of kevent is the best (or maybe only) way to show that. epoll is probably the easiest of the ones we have, so I'd suggest starting with it. inotify will be a lot harder, but we'll need that aswell. the kevent inode hooks you had in your earlier patches will never ever get in.
Was this clear enough?
Re: [take9 2/2] kevent: poll/select() notifications. Timer notifications.
Quoted 37 lines · Christoph Hellwig
Quoted 6 lines · Evgeniy Polyakov
Quoted 5 lines
If there's a really good reason we can keep things separate, but
"epoll and
kevent_polldiffers on some aspects"is not one :)
Quoted 9 lines
kevent_polluses hash table (actually it is kevent that uses table), locking is simpler and part of it is hidden in kevent core. Actuallykevent_pollis just a container allocator for poll wait queue. So epoll does not differ (except hash/tree and locking, which is based on locks for pathes which are shared in kevent with those ones which can be called from irq/bh context) from kevent +kevent_poll. And sincekevent_pollcan be not selected while epoll is always there (until embedded config is turned on), I recommend to have them both. Or always turn kevent on :)You mention a lot of implementation details that absoultely shouldn't matter to the userspace interface.
I might not have explained enough what the point behind all this is, so I'll try to explain it again:
- the fate of aio, inotify, epoll, etc shows we badly need a generic event mechnism that unifies event based interfaces of various subsystem. Only having a single mechanisms allows things like unified event loops and gives application progreammers the chance to learn that one interface for real and get it right.
- kevent looks like the right way to do this. but to show it can really archive this it needs to show it can do the things the existing event systems can do at least as good. reimplementing their user interfaces ontop of kevent is the best (or maybe only) way to show that. epoll is probably the easiest of the ones we have, so I'd suggest starting with it. inotify will be a lot harder, but we'll need that aswell. the kevent inode hooks you had in your earlier patches will never ever get in.
Quoted 1 line
Was this clear enough?
Sure, but if I say that it would sound like advertisement :) Some inotify notifications (inode create/remove) are implemented already in (dropped) FS notification patchset.
Signature · 1 line
Evgeniy Polyakov
Re: [take9 2/2] kevent: poll/select() notifications. Timer notifications.
In reply to Christoph HellwigQuoted 10 lines · Christoph Hellwig
Quoted 8 lines · Evgeniy Polyakov
poll/select() notifications. Timer notifications.
This patch includes generic poll/select and timer notifications.
kevent_pollworks simialr to epoll and has the same issues (callback is invoked not from internal state machine of the caller, but through process awake).
Quoted 7 lines
I'm not a big fan of duplicating code over and over. kevent is a candidate for a generic event devlivery mechanisms which is a _very_ good thing. But starting that system by duplicating existing functionality is not very nice.
What speaks against a patch the recplaces the epoll core by something that build on kevent while still supporting the epoll interface as a compatibility shim?
Sorry, I'm catching up with a huge post-vacation backlog, so I didn't have the time to look at the source code. But, if kevent performance is same or better, and the external epoll interface is fully supported, than I think the shim layer idea is a good one. Provided the shim being smaller than eventpoll.c :)
- Davide
Re: [take9 1/2] kevent: Core files.
In reply to Evgeniy Polyakovinclude/linux/kevent.h line 2
Quoted 8 lines
include/linux/kevent.h new file +2
-0@@ -0,0 +1,310 @@ +/* +* kevent.h
Please don't put filenames in the top of file block comments. They're redudant and as history shows out of date far too often.
include/linux/kevent.h line 126
Quoted 1 line
include/linux/kevent.h new file +1
-0+#ifdef __KERNEL__
Please split the user/kernel ABI and kernel implementation details into two different headers. That way we don't have to run unifdef as part of the user headers generation process and it's much cleaner what bit is a kernel implementation details and what's the public ABI.
include/linux/kevent.h line 158
Quoted 3 lines
include/linux/kevent.h new file +3
-0+#define KEVENT_READY 0x1 +#define KEVENT_STORAGE 0x2 +#define KEVENT_USER 0x4
Please use enums here.
include/linux/kevent.h line 180
Quoted 4 lines
include/linux/kevent.h new file +4
-0+void *priv;/* Private data for different storages. +* poll()/select storage has a list of wait_queue_t containers +* for each ->poll() { poll_wait()' } here. +*/
Please try to avoid spilling over the 80 chars limit. In this case it's easy, just put the comment before the field beeing documented.
include/linux/kevent.h line 183
Quoted 1 line
include/linux/kevent.h new file +1
-0+extern struct kevent_callbacks kevent_registered_callbacks[];
Having global arrays is not very nice. Any chance this could be hidden behind proper accessor functions?
Quoted 5 lines
include/linux/kevent.h new file +5
-0+#ifdef CONFIG_KEVENT_INODE +void kevent_inode_notify (struct inode *inode,u32 event); +void kevent_inode_notify_parent (struct dentry *dentry,u32 event); +void kevent_inode_remove (struct inode *inode); +#else
include/linux/kevent.h line 255
Quoted 10 lines
include/linux/kevent.h new file +10
-0+static inline void kevent_inode_notify (struct inode *inode,u32 event) +{ +} +static inline void kevent_inode_notify_parent (struct dentry *dentry,u32 event) +{ +} +static inline void kevent_inode_remove (struct inode *inode) +{ +} +#endif /* CONFIG_KEVENT_INODE */
The code implementing these prototypes doesn't exist.
include/linux/kevent.h line 267
Quoted 12 lines
include/linux/kevent.h new file +12
-0+#ifdef CONFIG_KEVENT_SOCKET +#ifdef CONFIG_LOCKDEP +void kevent_socket_reinit (struct socket *sock); +void kevent_sk_reinit (struct sock *sk); +#else +static inline void kevent_socket_reinit (struct socket *sock) +{ +} +static inline void kevent_sk_reinit (struct sock *sk) +{ +} +#endif
Dito. Please clean the header from all this dead code.
include/linux/kevent.h line 45
Quoted 7 lines
include/linux/kevent.h new file +7
-0+int kevent_storage_init (void *origin,struct kevent_storage *st) +{ +spin_lock_init (&st->lock); + st->origin = origin; +INIT_LIST_HEAD (&st->list); +return 0 ; +}
Why does this need a return value?
Quoted 5 lines
include/linux/kevent.h new file +5
-0+int kevent_sys_init (void ) +{ +int i; + + kevent_cache = kmem_cache_create("kevent_cache",
include/linux/kevent.h line 60
Quoted 10 lines
include/linux/kevent.h new file +10
-0+ sizeof(struct kevent), 0, SLAB_PANIC, NULL, NULL); + +for (i=0 ; i<ARRAY_SIZE (kevent_registered_callbacks); ++i) { +struct kevent_callbacks *c = &kevent_registered_callbacks[i]; + + c->callback = c->enqueue = c->dequeue =NULL ; + } + +return 0 ; +}
Please make this an initcall in this file and make sure it's linked before kevent_users.c
include/linux/kevent.h line 64
Quoted 4 lines
include/linux/kevent.h new file +4
-0+static int kevent_user_open (struct inode *,struct file *); +static int kevent_user_release (struct inode *,struct file *); +static unsigned int kevent_user_poll (struct file *,struct poll_table_struct *); +static int kevent_user_mmap (struct file *,struct vm_area_struct *);
Could you reorder the file so these forward-declaring prototypes aren't needed?
include/linux/kevent.h line 65
Quoted 1 line
include/linux/kevent.h new file +1
-0+for (i=0 ; i<ARRAY_SIZE (u->kevent_list); ++i)
for (i = 0; i < ARRAY_SIZE(u->kevent_list); i++)
include/linux/kevent.h line 78
Quoted 13 lines
include/linux/kevent.h new file +13
-0+static struct page *kevent_user_nopage (struct vm_area_struct *vma,unsigned long addr,int *type) +{ +struct kevent_user *u = vma->vm_file->private_data; +unsigned long off = (addr - vma->vm_start)/PAGE_SIZE ; +unsigned int pnum =ALIGN (KEVENT_MAX_EVENTS *sizeof (struct mukevent ) +sizeof (unsigned int ),PAGE_SIZE )/PAGE_SIZE ; + +if (type) + *type =VM_FAULT_MINOR ; + +if (off >= pnum) + goto err_out_sigbus; + + u->pring[off] =__get_free_page (GFP_KERNEL );
So we have a pagefault handler that allocates pages.
Quoted 6 lines
include/linux/kevent.h new file +6
-0+static int kevent_user_mmap (struct file *file,struct vm_area_struct *vma) +{ +unsigned long start = vma->vm_start; +struct kevent_user *u = file->private_data; + +if (vma->vm_flags &VM_WRITE )
include/linux/kevent.h line 94
Quoted 10 lines
include/linux/kevent.h new file +10
-0+return -EPERM ; + + vma->vm_page_prot =pgprot_noncached (vma->vm_page_prot); + vma->vm_ops = &kevent_user_vm_ops; + vma->vm_flags |=VM_RESERVED ; + vma->vm_file = file; + +if (remap_pfn_range (vma, start,virt_to_phys ((void *)u->pring[0 ]),PAGE_SIZE , + vma->vm_page_prot)) +return -EFAULT ;
but you always map the first page. This model sounds odd and rather confusing. Do we really need to avoid of the cost of the pagefault just for the special first page?
If so please at least use vm_insert_page() instead of remap_pfn_range().
Quoted 6 lines
include/linux/kevent.h new file +6
-0+#if 0 +static inline unsigned int kevent_user_hash (struct ukevent *uk) +{ +unsigned int h = (uk->user[0 ] ^ uk->user[1 ]) ^ (uk->id.raw[0 ] ^ uk->id.raw[1 ]); + + h = (((h >>16 ) &0xffff ) ^ (h &0xffff )) &0xffff ;
include/linux/kevent.h line 110
Quoted 10 lines
include/linux/kevent.h new file +10
-0+ h = (((h >>8 ) &0xff ) ^ (h &0xff )) &KEVENT_HASH_MASK ; + + return h; +} +#else +static inline unsigned int kevent_user_hash (struct ukevent *uk) +{ +return jhash_1word (uk->id.raw[0 ],0 ) &KEVENT_HASH_MASK ; +} +#endif
Please remove that #if 0 code.
Quoted 11 lines
include/linux/kevent.h new file +11
-0+static int kevent_ctl_process (struct file *file,unsigned int cmd,unsigned int num,void __user *arg) +{ +int err; +struct kevent_user *u = file->private_data; + +if (!u || num >KEVENT_MAX_EVENTS ) +return -EINVAL ; + +switch (cmd) { +case KEVENT_CTL_ADD : + err =kevent_user_ctl_add (u, num, arg);
include/linux/kevent.h line 131
Quoted 10 lines
include/linux/kevent.h new file +10
-0+break ; +case KEVENT_CTL_REMOVE : + err =kevent_user_ctl_remove (u, num, arg); +break ; +case KEVENT_CTL_MODIFY : + err =kevent_user_ctl_modify (u, num, arg); +break ; +default : + err = -EINVAL ; +break ;
We were rather against these kind of odd multiplexers in the past. For these three we at least have a common type beeing passed down so there's not compat handling problem, but I'm still not very happy with it..
include/linux/kevent.h line 138
Quoted 7 lines
include/linux/kevent.h new file +7
-0+asmlinkage long sys_kevent_ctl (int fd,unsigned int cmd,unsigned int num,void __user *arg) +{ +int err = -EINVAL ; +struct file *file; + +if (cmd ==KEVENT_CTL_INIT ) +return kevent_ctl_init ();
This one on the other hand is plain wrong. At least it should be a separate syscall. But looking at the code I don't quite understand why you need a syscall at all, why can't kevent be implemented as a cloning chardevice (on where every open allocates a new structure and stores it into file->private_data?)
Re: [take9 1/2] kevent: Core files.
Quoted 11 lines · Christoph Hellwig
include/linux/kevent.h line 2
Quoted 8 lines
include/linux/kevent.h new file +2
-0@@ -0,0 +1,310 @@ +/* +* kevent.h Please don't put filenames in the top of file block comments. They're redudant and as history shows out of date far too often.
Ok.
Quoted 6 lines
include/linux/kevent.h line 126
Quoted 1 line
include/linux/kevent.h new file +1
-0+#ifdef __KERNEL__Please split the user/kernel ABI and kernel implementation details into two different headers. That way we don't have to run unifdef as part of the user headers generation process and it's much cleaner what bit is a kernel implementation details and what's the public ABI.
ok.
Quoted 5 lines
include/linux/kevent.h line 158
Quoted 3 lines
include/linux/kevent.h new file +3
-0+#define KEVENT_READY 0x1 +#define KEVENT_STORAGE 0x2 +#define KEVENT_USER 0x4Please use enums here.
I used, but I was sugested to use define in some previous releases :)
Quoted 7 lines
include/linux/kevent.h line 180
Quoted 4 lines
include/linux/kevent.h new file +4
-0+void *priv;/* Private data for different storages. +* poll()/select storage has a list of wait_queue_t containers +* for each ->poll() { poll_wait()' } here. +*/ Please try to avoid spilling over the 80 chars limit. In this case it's easy, just put the comment before the field beeing documented.
Ok.
Quoted 4 lines
include/linux/kevent.h line 183
Quoted 1 line
include/linux/kevent.h new file +1
-0+extern struct kevent_callbacks kevent_registered_callbacks[];Having global arrays is not very nice. Any chance this could be hidden behind proper accessor functions?
Ok.
Quoted 9 lines
Quoted 9 lines
include/linux/kevent.h new file +9
-0+#ifdef CONFIG_KEVENT_INODE +void kevent_inode_notify (struct inode *inode,u32 event); +void kevent_inode_notify_parent (struct dentry *dentry,u32 event); +void kevent_inode_remove (struct inode *inode); +#else +static inline void kevent_inode_notify (struct inode *inode,u32 event) +{ +} +static inline void kevent_inode_notify_parent (struct dentry *dentry,u32 event)
Quoted 8 lines
include/linux/kevent.h line 255
Quoted 6 lines
include/linux/kevent.h new file +6
-0+{ +} +static inline void kevent_inode_remove (struct inode *inode) +{ +} +#endif /* CONFIG_KEVENT_INODE */ The code implementing these prototypes doesn't exist.
It exist, it was suggested by you to not include into patchset right now, so this file is not yet updated to not contain AIO stuff.
Quoted 15 lines
include/linux/kevent.h line 267
Quoted 12 lines
include/linux/kevent.h new file +12
-0+#ifdef CONFIG_KEVENT_SOCKET +#ifdef CONFIG_LOCKDEP +void kevent_socket_reinit (struct socket *sock); +void kevent_sk_reinit (struct sock *sk); +#else +static inline void kevent_socket_reinit (struct socket *sock) +{ +} +static inline void kevent_sk_reinit (struct sock *sk) +{ +} +#endif Dito. Please clean the header from all this dead code.
Quoted 9 lines
include/linux/kevent.h line 45
Quoted 7 lines
include/linux/kevent.h new file +7
-0+int kevent_storage_init (void *origin,struct kevent_storage *st) +{ +spin_lock_init (&st->lock); + st->origin = origin; +INIT_LIST_HEAD (&st->list); +return 0 ; +}Why does this need a return value?
Initialization in general can fail, this one can not, but I prefer to reserve a second plan.
Quoted 10 lines
Quoted 10 lines
include/linux/kevent.h new file +10
-0+int kevent_sys_init (void ) +{ +int i; + + kevent_cache =kmem_cache_create ("kevent_cache" , +sizeof (struct kevent ),0 ,SLAB_PANIC ,NULL ,NULL ); + +for (i=0 ; i<ARRAY_SIZE (kevent_registered_callbacks); ++i) { +struct kevent_callbacks *c = &kevent_registered_callbacks[i]; +
Quoted 8 lines
include/linux/kevent.h line 60
Quoted 5 lines
include/linux/kevent.h new file +5
-0+ c->callback = c->enqueue = c->dequeue =NULL ; + } + +return 0 ; +}Please make this an initcall in this file and make sure it's linked before
kevent_users.c
Ok.
Quoted 7 lines
include/linux/kevent.h line 64
Quoted 4 lines
include/linux/kevent.h new file +4
-0+static int kevent_user_open (struct inode *,struct file *); +static int kevent_user_release (struct inode *,struct file *); +static unsigned int kevent_user_poll (struct file *,struct poll_table_struct *); +static int kevent_user_mmap (struct file *,struct vm_area_struct *);Could you reorder the file so these forward-declaring prototypes aren't needed?
I prefer structures to be placed at the begining of the file, so it requires forward declaration.
But there is no sttrong feeling about that, so I will put it at the end.
Quoted 3 lines
include/linux/kevent.h line 65
Quoted 1 line
include/linux/kevent.h new file +1
-0+for (i=0 ; i<ARRAY_SIZE (u->kevent_list); ++i)for (i = 0; i < ARRAY_SIZE(u->kevent_list); i++)
Ugh, no. It reduces readability due to exessive number of spaces.
Quoted 7 lines
Quoted 7 lines
include/linux/kevent.h new file +7
-0+static struct page *kevent_user_nopage (struct vm_area_struct *vma,unsigned long addr,int *type) +{ +struct kevent_user *u = vma->vm_file->private_data; +unsigned long off = (addr - vma->vm_start)/PAGE_SIZE ; +unsigned int pnum =ALIGN (KEVENT_MAX_EVENTS *sizeof (struct mukevent ) +sizeof (unsigned int ),PAGE_SIZE )/PAGE_SIZE ; + +if (type)
Quoted 8 lines
include/linux/kevent.h line 78
Quoted 6 lines
include/linux/kevent.h new file +6
-0+ *type =VM_FAULT_MINOR ; + +if (off >= pnum) + goto err_out_sigbus; + + u->pring[off] =__get_free_page (GFP_KERNEL );So we have a pagefault handler that allocates pages.
It is fixed in take10 patchset, which is why it was released.
Quoted 12 lines
Quoted 12 lines
include/linux/kevent.h new file +12
-0+static int kevent_user_mmap (struct file *file,struct vm_area_struct *vma) +{ +unsigned long start = vma->vm_start; +struct kevent_user *u = file->private_data; + +if (vma->vm_flags &VM_WRITE ) +return -EPERM ; + + vma->vm_page_prot =pgprot_noncached (vma->vm_page_prot); + vma->vm_ops = &kevent_user_vm_ops; + vma->vm_flags |=VM_RESERVED ; + vma->vm_file = file;
Quoted 8 lines
include/linux/kevent.h line 94
Quoted 4 lines
include/linux/kevent.h new file +4
-0+ +if (remap_pfn_range (vma, start,virt_to_phys ((void *)u->pring[0 ]),PAGE_SIZE , + vma->vm_page_prot)) +return -EFAULT ;but you always map the first page. This model sounds odd and rather confusing. Do we really need to avoid of the cost of the pagefault just for the special first page?
->nopage() is fixed in take10 patchset to return next one. Number of pages can grow with te time until limit (which is quite high, and I was suggested to not allocate them all at startup).
Quoted 1 line
If so please at least use
vm_insert_page()instead ofremap_pfn_range().
Ok.
Quoted 10 lines
Quoted 10 lines
include/linux/kevent.h new file +10
-0+#if 0 +static inline unsigned int kevent_user_hash (struct ukevent *uk) +{ +unsigned int h = (uk->user[0 ] ^ uk->user[1 ]) ^ (uk->id.raw[0 ] ^ uk->id.raw[1 ]); + + h = (((h >>16 ) &0xffff ) ^ (h &0xffff )) &0xffff ; + h = (((h >>8 ) &0xff ) ^ (h &0xff )) &KEVENT_HASH_MASK ; + +return h; +}
Quoted 8 lines
include/linux/kevent.h line 110
Quoted 6 lines
include/linux/kevent.h new file +6
-0+#else +static inline unsigned int kevent_user_hash (struct ukevent *uk) +{ +return jhash_1word (uk->id.raw[0 ],0 ) &KEVENT_HASH_MASK ; +} +#endif Please remove that #if 0 code.
Ok.
Quoted 17 lines
+static int
kevent_ctl_process(struct file *file, unsigned int cmd, unsigned int num, void __user *arg)Quoted 16 lines
include/linux/kevent.h new file +16
-0+{ +int err; +struct kevent_user *u = file->private_data; + +if (!u || num >KEVENT_MAX_EVENTS ) +return -EINVAL ; + +switch (cmd) { +case KEVENT_CTL_ADD : + err =kevent_user_ctl_add (u, num, arg); +break ; +case KEVENT_CTL_REMOVE : + err =kevent_user_ctl_remove (u, num, arg); +break ; +case KEVENT_CTL_MODIFY : + err =kevent_user_ctl_modify (u, num, arg);
Quoted 8 lines
include/linux/kevent.h line 130
Quoted 4 lines
include/linux/kevent.h new file +4
-0+break ; +default : + err = -EINVAL ; +break ;We were rather against these kind of odd multiplexers in the past. For these three we at least have a common type beeing passed down so there's not compat handling problem, but I'm still not very happy with it..
I use one syscall for add/remove/modify, so it requires multiplexer.
Quoted 5 lines
Quoted 5 lines
include/linux/kevent.h new file +5
-0+asmlinkage long sys_kevent_ctl (int fd,unsigned int cmd,unsigned int num,void __user *arg) +{ +int err = -EINVAL ; +struct file *file; +
Quoted 8 lines
include/linux/kevent.h line 137
Quoted 2 lines
include/linux/kevent.h new file +2
-0+if (cmd ==KEVENT_CTL_INIT ) +return kevent_ctl_init ();This one on the other hand is plain wrong. At least it should be a separate syscall. But looking at the code I don't quite understand why you need a syscall at all, why can't kevent be implemented as a cloning chardevice (on where every open allocates a new structure and stores it into
file->private_data?)
That requires separate syscall.
I created a char device in first releases and was forced to not use it at all.
Signature · 1 line
Evgeniy Polyakov
Re: [take9 1/2] kevent: Core files.
Quoted 5 lines
Quoted 3 lines
kernel/kevent/kevent_user.c line 217
Quoted 2 lines
kernel/kevent/kevent_user.c new file +1
-0+for (i=0 ; i<ARRAY_SIZE (u->kevent_list); ++i)for (i = 0; i < ARRAY_SIZE(u->kevent_list); i++)Ugh, no. It reduces readability due to exessive number of spaces.
Ihavetoverystronglydisagree.
- z
Re: [take9 1/2] kevent: Core files.
Quoted 7 lines · Zach Brown
Quoted 5 lines
Quoted 3 lines
kernel/kevent/kevent_user.c line 217
Quoted 2 lines
kernel/kevent/kevent_user.c new file +1
-0+for (i=0 ; i<ARRAY_SIZE (u->kevent_list); ++i)for (i = 0; i < ARRAY_SIZE(u->kevent_list); i++)Ugh, no. It reduces readability due to exessive number of spaces.
Ihavetoverystronglydisagree.
W e l l , i f y o u i n s i s t a n d a b s o l u t e l y s u r e.
Quoted 1 line
- z
Signature · 1 line
Evgeniy Polyakov
David Miller
maintainerRe: [take9 1/2] kevent: Core files.
In reply to Zach BrownFrom: Zach Brown <[email protected]>
Date: Wed, 16 Aug 2006 11:08:41 -0700
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
kernel/kevent/kevent_user.c line 217
Quoted 1 line
kernel/kevent/kevent_user.c new file +1
-0+for (i=0 ; i<ARRAY_SIZE (u->kevent_list); ++i)for (i = 0; i < ARRAY_SIZE(u->kevent_list); i++)Ugh, no. It reduces readability due to exessive number of spaces.
Ihavetoverystronglydisagree.
Metoo. :-)
Spaces help humans parse out the syntactic structure of multi-token expressions.
Re: [take9 1/2] kevent: Core files.
Quoted 11 lines · David Miller
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
kernel/kevent/kevent_user.c line 217
Quoted 1 line
kernel/kevent/kevent_user.c new file +1
-0+for (i=0 ; i<ARRAY_SIZE (u->kevent_list); ++i)for (i = 0; i < ARRAY_SIZE(u->kevent_list); i++)Ugh, no. It reduces readability due to exessive number of spaces.
Ihavetoverystronglydisagree.
Metoo. :-)
Spaces help humans parse out the syntactic structure of multi-token expressions.
There is an anecdote:
in a near future, when world crisis killed economy,
russain scientists found a way to make time machine, so they get some dictator from the past and ask him how to improve a situation. He quickly answers that it is only needed to shoot all opposition, kill the freedom and redraw Kremlin and Red Square into blue colour. People wonder "such tragic steps, so much blood, but why Red Square?" Dictator answers: well, if there are no objections about other issues, we will not change Red Square.
I'm telling it just because I would like to know if there are any issues which must be fixed in the tomorrow patchset except mentioned in previous e-mails... :)
Signature · 1 line
Evgeniy Polyakov
Re: [take9 1/2] kevent: Core files.
In reply to Evgeniy PolyakovQuoted 7 lines
Quoted 5 lines
include/linux/kevent.h line 158
Quoted 3 lines
include/linux/kevent.h new file +3
-0+#define KEVENT_READY 0x1 +#define KEVENT_STORAGE 0x2 +#define KEVENT_USER 0x4Please use enums here.
I used, but I was sugested to use define in some previous releases :)
defines make some sense for userspace-visible ABIs because then people can test for features with ifdef. It doesn't make any sense for constants that are used purely in-kernel. For those enums make more sense because you can for example looks at the symbolic names with a debugger.
Quoted 5 lines
Quoted 3 lines
We were rather against these kind of odd multiplexers in the past. For these three we at least have a common type beeing passed down so there's not compat handling problem, but I'm still not very happy with it..
I use one syscall for add/remove/modify, so it requires multiplexer.
I noticed that you do it, but it's not exactly considered a nice design.
Quoted 14 lines
Quoted 5 lines
Quoted 5 lines
kernel/kevent/kevent_user.c new file +5
-0+asmlinkage long sys_kevent_ctl (int fd,unsigned int cmd,unsigned int num,void __user *arg) +{ +int err = -EINVAL ; +struct file *file; +Quoted 8 lines
kernel/kevent/kevent_user.c line 944
Quoted 2 lines
kernel/kevent/kevent_user.c new file +2
-0+if (cmd ==KEVENT_CTL_INIT ) +return kevent_ctl_init ();This one on the other hand is plain wrong. At least it should be a separate syscall. But looking at the code I don't quite understand why you need a syscall at all, why can't kevent be implemented as a cloning chardevice (on where every open allocates a new structure and stores it into
file->private_data?)
Quoted 1 line
That requires separate syscall.
Yes, it requires a separate syscall.
Quoted 2 lines
I created a char device in first releases and was forced to not use it at all.
Do you have a reference to it? In this case a char devices makes a lot of sense because you get a filedescriptor and have operations only defined on it. In fact given that you have a multiplexer anyway there's really no point in adding a syscall for that aswell, you could rather use the existing and debugged ioctl() multiplexer. Sure, it's still not what we consider nice, but better than adding even more odd multiplexer syscalls.
Re: [take9 1/2] kevent: Core files.
Quoted 6 lines · Christoph Hellwig
Quoted 6 lines
Quoted 5 lines
include/linux/kevent.h line 158
Quoted 3 lines
include/linux/kevent.h new file +3
-0+#define KEVENT_READY 0x1 +#define KEVENT_STORAGE 0x2 +#define KEVENT_USER 0x4Please use enums here.
Quoted 6 lines
Quoted 1 line
I used, but I was sugested to use define in some previous releases :)
defines make some sense for userspace-visible ABIs because then people can test for features with ifdef. It doesn't make any sense for constants that are used purely in-kernel. For those enums make more sense because you can for example looks at the symbolic names with a debugger.
Enums are only usefull when value is increased with each new member by one.
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
We were rather against these kind of odd multiplexers in the past. For these three we at least have a common type beeing passed down so there's not compat handling problem, but I'm still not very happy with it..
I use one syscall for add/remove/modify, so it requires multiplexer.
I noticed that you do it, but it's not exactly considered a nice design.
There will be either several syscalls or multiplexer... I prefer to have one syscall and a lot of multiplexers inside.
Quoted 21 lines
Quoted 14 lines
Quoted 5 lines
Quoted 5 lines
kernel/kevent/kevent_user.c new file +5
-0+asmlinkage long sys_kevent_ctl (int fd,unsigned int cmd,unsigned int num,void __user *arg) +{ +int err = -EINVAL ; +struct file *file; +Quoted 8 lines
kernel/kevent/kevent_user.c line 944
Quoted 2 lines
kernel/kevent/kevent_user.c new file +2
-0+if (cmd ==KEVENT_CTL_INIT ) +return kevent_ctl_init ();This one on the other hand is plain wrong. At least it should be a separate syscall. But looking at the code I don't quite understand why you need a syscall at all, why can't kevent be implemented as a cloning chardevice (on where every open allocates a new structure and stores it into
file->private_data?)Quoted 1 line
That requires separate syscall.
Yes, it requires a separate syscall.
Quoted 2 lines
I created a char device in first releases and was forced to not use it at all.
Quoted 6 lines
Do you have a reference to it? In this case a char devices makes a lot of sense because you get a filedescriptor and have operations only defined on it. In fact given that you have a multiplexer anyway there's really no point in adding a syscall for that aswell, you could rather use the existing and debugged
ioctl()multiplexer. Sure, it's still not what we consider nice, but better than adding even more odd multiplexer syscalls.
Somewhere in february.
Here is link to initial anounce which used ioctl and raw char device and enums for all constants.
http://marc.theaimsgroup.com/?l=linux-netdev&m=113949344414464&w=2
Signature · 1 line
Evgeniy Polyakov
Re: [take9 1/2] kevent: Core files.
Quoted 7 lines · Evgeniy Polyakov
Quoted 4 lines
defines make some sense for userspace-visible ABIs because then people can test for features with ifdef. It doesn't make any sense for constants that are used purely in-kernel. For those enums make more sense because you can for example looks at the symbolic names with a debugger.
Enums are only usefull when value is increased with each new member by one.
No, they are not. Please search the lkml archives, this came up multiple times.
Quoted 2 lines
There will be either several syscalls or multiplexer... I prefer to have one syscall and a lot of multiplexers inside.
To make life for everyone to detangle the mess hard. Please at least try to follow existing design principles.
Quoted 10 lines
Quoted 9 lines
Quoted 2 lines
I created a char device in first releases and was forced to not use it at all.
Do you have a reference to it? In this case a char devices makes a lot of sense because you get a filedescriptor and have operations only defined on it. In fact given that you have a multiplexer anyway there's really no point in adding a syscall for that aswell, you could rather use the existing and debugged
ioctl()multiplexer. Sure, it's still not what we consider nice, but better than adding even more odd multiplexer syscalls.
Quoted 5 lines
Somewhere in february.
Here is link to initial anounce which used ioctl and raw char device and enums for all constants.http://marc.theaimsgroup.com/?l=linux-netdev&m=113949344414464&w=2
That thread only shows your patch but no comments to it. Do you have an url for the complaint about this design? And please include the author of it in the cc list of your reply so we can settle the arguments.
Re: [take9 1/2] kevent: Core files.
Quoted 11 lines · Christoph Hellwig
Quoted 7 lines · Evgeniy Polyakov
Quoted 4 lines
defines make some sense for userspace-visible ABIs because then people can test for features with ifdef. It doesn't make any sense for constants that are used purely in-kernel. For those enums make more sense because you can for example looks at the symbolic names with a debugger.
Enums are only usefull when value is increased with each new member by one.
No, they are not. Please search the lkml archives, this came up multiple times.
Enums when OR'ed and ANDed in theory can produce value out of the enum set.
And what is the difference between
#define A 1
#define B 2
#define C 4
and
enum {
A = 1,
B = 2,
C = 4,
}
?
Quoted 5 lines
Quoted 2 lines
There will be either several syscalls or multiplexer... I prefer to have one syscall and a lot of multiplexers inside.
To make life for everyone to detangle the mess hard. Please at least try to follow existing design principles.
I added as less as possible syscalls - control one read ones. The former can initialize/add/remove/modify kevents, the latter reads ready events.
Quoted 14 lines
Quoted 14 lines
Quoted 9 lines
Quoted 2 lines
I created a char device in first releases and was forced to not use it at all.
Do you have a reference to it? In this case a char devices makes a lot of sense because you get a filedescriptor and have operations only defined on it. In fact given that you have a multiplexer anyway there's really no point in adding a syscall for that aswell, you could rather use the existing and debugged
ioctl()multiplexer. Sure, it's still not what we consider nice, but better than adding even more odd multiplexer syscalls.Somewhere in february.
Here is link to initial anounce which used ioctl and raw char device and enums for all constants.
Quoted 5 lines
That thread only shows your patch but no comments to it. Do you have an url for the complaint about this design? And please include the author of it in the cc list of your reply so we can settle the arguments.
enum vs. defines were changed after David Miller's suggestion, since the whole network stack and epoll use defines extensively, and kevent was built for them in a first time.
I do not remember who suggested not to use char device, but I saw that inotify specially mention about ioct/syscall issues, old discussion about event handling mechanism somewhere in this thread:
http://uwsg.iu.edu/hypermail/linux/kernel/0010.3/0002.html
It looks a bit illogical to have epoll/poll syscalls and read data from char device and it's ioctls for network sockets completeness or timer notifications.
Signature · 1 line
Evgeniy Polyakov
Re: [take9 1/2] kevent: Core files.
On Mon, 2006-08-21 at 15:13 +0400, Evgeniy Polyakov wrote:
[...]
Quoted 1 line
And what is the difference between
As others already pointed out in this thread:
These are not seen by the C compiler.
Quoted 4 lines
#define A 1
#define B 2
#define C 4
and
These are known by the C compiler and thus usable/viewable in a debugger.
Quoted 6 lines
enum {
A = 1,
B = 2,
C = 4,
}
?
Bernd
Signature · 3 lines
Firmix Software GmbH http://www.firmix.at/
mobil: +43 664 4416156 fax: +43 1 7890849-55
Embedded Linux Development and ServicesRe: [take9 1/2] kevent: Core files.
Quoted 6 lines · Bernd Petrovitsch
On Mon, 2006-08-21 at 15:13 +0400, Evgeniy Polyakov wrote:
[...]Quoted 1 line
And what is the difference between
As others already pointed out in this thread:
Quoted 14 lines
These are not seen by the C compiler.
Quoted 4 lines
#define A 1
#define B 2
#define C 4
andThese are known by the C compiler and thus usable/viewable in a debugger.
Quoted 6 lines
enum {
A = 1,
B = 2,
C = 4,
}
?
:) And I pointed quite a few other issues about enums vs. defines. According to this one - no one wants to watch enums in debugger.
And, ugh:
(gdb) list
1 enum {
2 A = 1,
3 B = 2,
4 };
5
6 int main()
7 {
8 printf("%x\n", A | B);
9 }
(gdb) bre 8
Breakpoint 1 at 0x4004ac: file ./test.c, line 8.
(gdb) r
Starting program: /tmp/test
Breakpoint 1, main () at ./test.c:8
8 printf("%x\n", A | B);
(gdb) p A
No symbol "A" in current context.
Actually I completely do not care about define or enums, it is really silly dispute, I just do not want to rewrite bunch of code _again_ and then _again_ when someone decide that defines are better.
Signature · 1 line
Evgeniy Polyakov
Re: [take9 1/2] kevent: Core files.
On Mon, 2006-08-21 at 17:01 +0400, Evgeniy Polyakov wrote:
[ #define vs enum { } ]
Quoted 17 lines
And, ugh:
(gdb) list
1 enum {
2 A = 1,
3 B = 2,
4 };
5
6 intmain()
7 {
8printf("%x\n", A | B);
9 }
(gdb) bre 8
Breakpoint 1 at0x4004ac: file ./test.c, line 8.
(gdb) r
Starting program:/tmp/test
Quoted 4 lines
Breakpoint 1, main () at ./test.c:8
8printf("%x\n", A | B);
(gdb) p A
No symbol "A" in current context.
Oops, I stand corrected.
Quoted 3 lines
Actually I completely do not care about define or enums, it is really silly dispute, I just do not want to rewrite bunch of code _again_ and then _again_ when someone decide that defines are better.
ACK. Personally I also do not care that much - as long as it doesn't change with the phase of the moon.
And we probably do not want
---- snip ----
#ifdef CONFIG_I_LOVE_ENUMS
#define A 1
#define B 2
#define C 4
#else
enum {
A = 1,
B = 2,
C = 4,
};#endif
---- snip ----
either.
Bernd, shutting now up on this thread
Signature · 3 lines
Firmix Software GmbH http://www.firmix.at/
mobil: +43 664 4416156 fax: +43 1 7890849-55
Embedded Linux Development and ServicesDavid Miller
maintainerRe: [take9 1/2] kevent: Core files.
In reply to Evgeniy PolyakovFrom: Evgeniy Polyakov <[email protected]>
Date: Mon, 21 Aug 2006 17:01:21 +0400
Quoted 3 lines
Actually I completely do not care about define or enums, it is really silly dispute, I just do not want to rewrite bunch of code _again_ and then _again_ when someone decide that defines are better.
I totally agree.
What in the world is wrong with you people arguing over stuff like this?
If the goal is to discourage Evgeniy and his work, you might just get your wish if you keep up with this silly coding style and enumeration crap!
I can't even stand to read these kevent threads any longer, the sanity in them has long gone out the window.
Re: [take9 0/2] kevent: Generic event handling mechanism.
In reply to Evgeniy PolyakovQuoted 2 lines · Evgeniy Polyakov
Generic event handling mechanism.
Hi, I've just started looking into this, so some comments here first on the submission process:
- could you send new revisions of the patches in a new thread so one can easily find them?
- the patch split is not very nice, your first patch adds Makefile and Kconfig entries for files only in the second patch or not actually submitted at all, that's a big no-no.
Re: [take9 0/2] kevent: Generic event handling mechanism.
Quoted 9 lines · Christoph Hellwig
Quoted 2 lines · Evgeniy Polyakov
Generic event handling mechanism.
Hi, I've just started looking into this, so some comments here first on the submission process:
- could you send new revisions of the patches in a new thread so one can easily find them?
Ok.
Quoted 3 lines
- the patch split is not very nice, your first patch adds
MakefileandKconfigentries for files only in the second patch or not actually submitted at all, that's a big no-no.
It is done by scripts using list of files generated by git-diff, but I can reformat them to be in a way:
core files
poll/select
timer
any other
main Kconfig/Makefile
Kevent's makefile still contains all entries for files added later, is it a big problem right now?
I can split patches manually, but it would be much better to do it when decision about it's inclusion is made, and until review and feature addiotion process is not completed generate patches as is...
Signature · 1 line
Evgeniy Polyakov
Re: [take9 0/2] kevent: Generic event handling mechanism.
Quoted 3 lines
It is done by scripts using list of files generated by git-diff, but I can reformat them to be in a way:
Perhaps you should think about maintaining them as an explicit series of patches, say with quilt or mq, instead of as one repository that you try and cut up into separate patches.
- z
cover kevent: Generic event handling mechanism.
In reply to Evgeniy PolyakovGeneric event handling mechanism.
Changes from 'take9' patchset:
* fixed ->nopage method
Changes from 'take8' patchset:
* fixed mmap release bug
* use module_init() instead of late_initcall()
* use better structures for timer notifications
Changes from 'take7' patchset:
* new mmap interface (not tested, waiting for other changes to be acked)
- use nopage() method to dynamically substitue pages
- allocate new page for events only when new added kevent requres it
- do not use ugly index dereferencing, use structure instead
- reduced amount of data in the ring (id and flags), maximum 12 pages on x86 per kevent fd
Changes from 'take6' patchset:
* a lot of comments!
* do not use list poisoning for detection of the fact, that entry is in the list
* return number of ready kevents even if copy*user() fails
* strict check for number of kevents in syscall
* use ARRAY_SIZE for array size calculation
* changed superblock magic number
* use SLAB_PANIC instead of direct panic() call
* changed -E* return values
* a lot of small cleanups and indent fixes
Changes from 'take5' patchset:
* removed compilation warnings about unused wariables when lockdep is not turned on
* do not use internal socket structures, use appropriate (exported) wrappers instead
* removed default 1 second timeout
* removed AIO stuff from patchset
Changes from 'take4' patchset:
* use miscdevice instead of chardevice
* comments fixes
Changes from 'take3' patchset:
* removed serializing mutex from kevent_user_wait()
* moved storage list processing to RCU
* removed lockdep screaming - all storage locks are initialized in the same function, so it was learned to differentiate between various cases
* remove kevent from storage if is marked as broken after callback
* fixed a typo in mmaped buffer implementation which would end up in wrong index calcualtion
Changes from 'take2' patchset:
* split kevent_finish_user() to locked and unlocked variants
* do not use KEVENT_STAT ifdefs, use inline functions instead
* use array of callbacks of each type instead of each kevent callback initialization
* changed name of ukevent guarding lock
* use only one kevent lock in kevent_user for all hash buckets instead of per-bucket locks
* do not use kevent_user_ctl structure instead provide needed arguments as syscall parameters
* various indent cleanups
* added optimisation, which is aimed to help when a lot of kevents are being copied from userspace
* mapped buffer (initial) implementation (no userspace yet)
Changes from 'take1' patchset:
- rebased against 2.6.18-git tree
- removed ioctl controlling
- added new syscall kevent_get_events(int fd, unsigned int min_nr, unsigned int max_nr, unsigned int timeout, void __user *buf, unsigned flags)
- use old syscall kevent_ctl for creation/removing, modification and initial kevent initialization
- use mutuxes instead of semaphores
- added file descriptor check and return error if provided descriptor does not match kevent file operations
- various indent fixes
- removed aio_sendfile() declarations.
Thank you.
PATCH 1/2 kevent: Core files.
Core files.
This patch includes core kevent files:
- userspace controlling
- kernelspace interfaces
- initialization
- notification state machines
arch/i386/kernel/syscall_table.S +2 -0
@@ -317,3 +317,5 @@ ENTRY(sys_call_table) .long sys_tee /* 315 */ .long sys_vmsplice .long sys_move_pages + .long sys_kevent_get_events + .long sys_kevent_ctl
arch/x86_64/ia32/ia32entry.S +2 -0
@@ -713,4 +713,6 @@ #endif .quad sys_tee .quad compat_sys_vmsplice .quad compat_sys_move_pages + .quad sys_kevent_get_events + .quad sys_kevent_ctl ia32_syscall_end:
include/asm-i386/unistd.h +3 -1
@@ -323,10 +323,12 @@ #define __NR_sync_file_range 314#define __NR_tee 315#define __NR_vmsplice 316#define __NR_move_pages 317 +#define __NR_kevent_get_events 318 +#define __NR_kevent_ctl 319#ifdef __KERNEL__ -#define NR_syscalls 318 +#define NR_syscalls 320/* * user-visible error numbers are in the range -1 - -128: see
include/asm-x86_64/unistd.h +5 -1
@@ -619,10 +619,14 @@ #define __NR_vmsplice 278 __SYSCALL(__NR_vmsplice, sys_vmsplice)#define __NR_move_pages 279 __SYSCALL(__NR_move_pages, sys_move_pages) +#define __NR_kevent_get_events 280 +__SYSCALL(__NR_kevent_get_events, sys_kevent_get_events) +#define __NR_kevent_ctl 281 +__SYSCALL(__NR_kevent_ctl, sys_kevent_ctl)#ifdef __KERNEL__ -#define __NR_syscall_max __NR_move_pages +#define __NR_syscall_max __NR_kevent_ctl#ifndef __NO_STUBS
include/linux/kevent.h new file +310 -0
310 lines not on this page · raw message
include/linux/kevent_storage.h new file +11 -0
@@ -0,0 +1,11 @@ +#ifndef __KEVENT_STORAGE_H +#define __KEVENT_STORAGE_H + +struct kevent_storage +{ +void *origin;/* Originator's pointer, e.g. struct sock or struct file. Can be NULL. */ +struct list_head list;/* List of queued kevents. */ +spinlock_t lock;/* Protects users queue. */ +}; + +#endif /* __KEVENT_STORAGE_H */
include/linux/syscalls.h +3 -0
@@ -597,4 +597,7 @@ asmlinkage long sys_get_robust_list(intasmlinkage long sys_set_robust_list (struct robust_list_head __user *head,size_t len); +asmlinkage long sys_kevent_get_events (int ctl_fd,unsigned int min,unsigned int max, +unsigned int timeout,void __user *buf,unsigned flags); +asmlinkage long sys_kevent_ctl (int ctl_fd,unsigned int cmd,unsigned int num,void __user *buf);#endif
init/Kconfig +2 -0
@@ -218,6 +218,8 @@ config AUDITSYSCALL such as SELinux. To use audit's filesystem watch feature, please ensure that INOTIFY is configured. +source "kernel/kevent/Kconfig" + config IKCONFIG bool "Kernel .config support" ---help---
kernel/Makefile +1 -0
@@ -47,6 +47,7 @@ obj-$(CONFIG_DETECT_SOFTLOCKUP) += softl obj-$(CONFIG_GENERIC_HARDIRQS) += irq/ obj-$(CONFIG_SECCOMP) += seccomp.o obj-$(CONFIG_RCU_TORTURE_TEST) += rcutorture.o +obj-$(CONFIG_KEVENT) += kevent/ obj-$(CONFIG_RELAY) += relay.o obj-$(CONFIG_TASK_DELAY_ACCT) += delayacct.o obj-$(CONFIG_TASKSTATS) += taskstats.o
kernel/kevent/Kconfig new file +59 -0
59 lines not on this page · raw message
kernel/kevent/Makefile new file +7 -0
@@ -0,0 +1,7 @@ +obj-y := kevent.o kevent_user.o +obj-$(CONFIG_KEVENT_SOCKET) += kevent_socket.o +obj-$(CONFIG_KEVENT_INODE) += kevent_inode.o +obj-$(CONFIG_KEVENT_TIMER) += kevent_timer.o +obj-$(CONFIG_KEVENT_POLL) += kevent_poll.o +obj-$(CONFIG_KEVENT_NAIO) += kevent_naio.o +obj-$(CONFIG_KEVENT_AIO) += kevent_aio.o
kernel/kevent/kevent.c new file +249 -0
249 lines not on this page · raw message
kernel/kevent/kevent_user.c new file +1002 -0
1,002 lines not on this page · raw message
kernel/sys_ni.c +3 -0
@@ -122,6 +122,9 @@ cond_syscall(ppc_rtas);cond_syscall (sys_spu_run);cond_syscall (sys_spu_create); +cond_syscall (sys_kevent_get_events); +cond_syscall (sys_kevent_ctl); +/* mmu depending weak syscall entries */ cond_syscall (sys_mprotect);cond_syscall (sys_msync);
PATCH 2/2 kevent: poll/select() notifications. Timer notifications.
poll/select() notifications. Timer notifications.
This patch includes generic poll/select and timer notifications.
kevent_poll works simialr to epoll and has the same issues (callback is invoked not from internal state machine of the caller, but through process awake).
Timer notifications can be used for fine grained per-process time management, since interval timers are very inconvenient to use, and they are limited.
kernel/kevent/kevent_poll.c new file +220 -0
@@ -0,0 +1,220 @@ +/* +* kevent_poll.c +* +* 2006 Copyright (c) Evgeniy Polyakov <[email protected]> +* All rights reserved. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#include <linux/kernel.h> +#include <linux/types.h> +#include <linux/list.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/timer.h> +#include <linux/file.h> +#include <linux/kevent.h> +#include <linux/poll.h> +#include <linux/fs.h> + +static kmem_cache_t *kevent_poll_container_cache; +static kmem_cache_t *kevent_poll_priv_cache; + +struct kevent_poll_ctl +{ +struct poll_table_struct pt; +struct kevent *k; +}; + +struct kevent_poll_wait_container +{ +struct list_head container_entry; +wait_queue_head_t *whead; +wait_queue_t wait; +struct kevent *k; +}; + +struct kevent_poll_private +{ +struct list_head container_list; +spinlock_t container_lock; +}; + +static int kevent_poll_enqueue (struct kevent *k); +static int kevent_poll_dequeue (struct kevent *k); +static int kevent_poll_callback (struct kevent *k); + +static int kevent_poll_wait_callback (wait_queue_t *wait, +unsigned mode,int sync,void *key) +{ +struct kevent_poll_wait_container *cont = + container_of(wait, struct kevent_poll_wait_container, wait); +struct kevent *k = cont->k; +struct file *file = k->st->origin; +u32 revents; + + revents = file->f_op->poll (file,NULL ); + +kevent_storage_ready (k->st,NULL , revents); + +return 0 ; +} + +static void kevent_poll_qproc (struct file *file,wait_queue_head_t *whead, +struct poll_table_struct *poll_table) +{ +struct kevent *k = + container_of(poll_table, struct kevent_poll_ctl, pt)->k; +struct kevent_poll_private *priv = k->priv; +struct kevent_poll_wait_container *cont; +unsigned long flags; + + cont =kmem_cache_alloc (kevent_poll_container_cache,SLAB_KERNEL ); +if (!cont) { +kevent_break (k); +return ; + } + + cont->k = k; +init_waitqueue_func_entry (&cont->wait, kevent_poll_wait_callback); + cont->whead = whead; + +spin_lock_irqsave (&priv->container_lock, flags); +list_add_tail (&cont->container_entry, &priv->container_list); +spin_unlock_irqrestore (&priv->container_lock, flags); + +add_wait_queue (whead, &cont->wait); +} + +static int kevent_poll_enqueue (struct kevent *k) +{ +struct file *file; +int err, ready =0 ; +unsigned int revents; +struct kevent_poll_ctl ctl; +struct kevent_poll_private *priv; + + file =fget (k->event.id.raw[0 ]); +if (!file) +return -ENODEV ; + + err = -EINVAL ; +if (!file->f_op || !file->f_op->poll) + goto err_out_fput; + + err = -ENOMEM ; + priv =kmem_cache_alloc (kevent_poll_priv_cache,SLAB_KERNEL ); +if (!priv) + goto err_out_fput; + +spin_lock_init (&priv->container_lock); +INIT_LIST_HEAD (&priv->container_list); + + k->priv = priv; + + ctl.k = k; +init_poll_funcptr (&ctl.pt, &kevent_poll_qproc); + + err =kevent_storage_enqueue (&file->st, k); +if (err) + goto err_out_free; + + revents = file->f_op->poll (file, &ctl.pt); +if (revents & k->event.event) { + ready =1 ; +kevent_poll_dequeue (k); + } + +return ready; + +err_out_free: +kmem_cache_free (kevent_poll_priv_cache, priv); +err_out_fput: +fput (file); +return err; +} + +static int kevent_poll_dequeue (struct kevent *k) +{ +struct file *file = k->st->origin; +struct kevent_poll_private *priv = k->priv; +struct kevent_poll_wait_container *w, *n; +unsigned long flags; + +kevent_storage_dequeue (k->st, k); + +spin_lock_irqsave (&priv->container_lock, flags); +list_for_each_entry_safe (w, n, &priv->container_list, container_entry) { +list_del (&w->container_entry); +remove_wait_queue (w->whead, &w->wait); +kmem_cache_free (kevent_poll_container_cache, w); + } +spin_unlock_irqrestore (&priv->container_lock, flags); + +kmem_cache_free (kevent_poll_priv_cache, priv); + k->priv =NULL ; + +fput (file); + +return 0 ; +} + +static int kevent_poll_callback (struct kevent *k) +{ +struct file *file = k->st->origin; +unsigned int revents = file->f_op->poll (file,NULL ); +return (revents & k->event.event); +} + +static int __init kevent_poll_sys_init (void ) +{ +struct kevent_callbacks *pc = &kevent_registered_callbacks[KEVENT_POLL ]; + + kevent_poll_container_cache =kmem_cache_create ("kevent_poll_container_cache" , +sizeof (struct kevent_poll_wait_container ),0 ,0 ,NULL ,NULL ); +if (!kevent_poll_container_cache) { +printk (KERN_ERR "Failed to create kevent poll container cache.\n" ); +return -ENOMEM ; + } + + kevent_poll_priv_cache =kmem_cache_create ("kevent_poll_priv_cache" , +sizeof (struct kevent_poll_private ),0 ,0 ,NULL ,NULL ); +if (!kevent_poll_priv_cache) { +printk (KERN_ERR "Failed to create kevent poll private data cache.\n" ); +kmem_cache_destroy (kevent_poll_container_cache); + kevent_poll_container_cache =NULL ; +return -ENOMEM ; + } + + pc->enqueue = &kevent_poll_enqueue; + pc->dequeue = &kevent_poll_dequeue; + pc->callback = &kevent_poll_callback; + +printk (KERN_INFO "Kevent poll()/select() subsystem has been initialized.\n" ); +return 0 ; +} + +static struct lock_class_key kevent_poll_key; + +void kevent_poll_reinit (struct file *file) +{ +lockdep_set_class (&file->st.lock, &kevent_poll_key); +} + +static void __exit kevent_poll_sys_fini(void) +{ +kmem_cache_destroy (kevent_poll_priv_cache); +kmem_cache_destroy (kevent_poll_container_cache); +} + +module_init (kevent_poll_sys_init); +module_exit (kevent_poll_sys_fini);
kernel/kevent/kevent_timer.c new file +108 -0
108 lines not on this page · raw message
Re: [take10 1/2] kevent: Core files.
In reply to Evgeniy Polyakov+void kevent_user_ring_add_event(struct kevent *k) +{
+ unsigned int pidx, off;
+ struct kevent_mring *ring, *copy_ring;
+
+ ring = (struct kevent_mring *)k->user->pring[0];
+
+ pidx = ring->index/KEVENTS_ON_PAGE;
+ off = ring->index%KEVENTS_ON_PAGE;
+
+ copy_ring = (struct kevent_mring *)k->user->pring[pidx];
+
+ copy_ring->event[off].id.raw[0] = k->event.id.raw[0];
+ copy_ring->event[off].id.raw[1] = k->event.id.raw[1];
+ copy_ring->event[off].ret_flags = k->event.ret_flags;
+
+ if (++ring->index >= KEVENT_MAX_EVENTS)
+ ring->index = 0;
+}
Can you assume that the page at pidx is already allocated and why?
--Mika
Re: [take10 1/2] kevent: Core files.
Quoted 20 lines · Mika Penttilä
kernel/kevent/kevent_user.c new file +19
-0+void kevent_user_ring_add_event (struct kevent *k) +{ +unsigned int pidx, off; +struct kevent_mring *ring, *copy_ring; + + ring = (struct kevent_mring *)k->user->pring[0 ]; + + pidx = ring->index/KEVENTS_ON_PAGE ; + off = ring->index%KEVENTS_ON_PAGE ; + + copy_ring = (struct kevent_mring *)k->user->pring[pidx]; + + copy_ring->event[off].id.raw[0 ] = k->event.id.raw[0 ]; + copy_ring->event[off].id.raw[1 ] = k->event.id.raw[1 ]; + copy_ring->event[off].ret_flags = k->event.ret_flags; + +if (++ring->index >=KEVENT_MAX_EVENTS ) + ring->index =0 ; +}
Quoted 1 line
Can you assume that the page at pidx is already allocated and why?
It is checked and allocated if needed in kevent_user_ring_grow(), which is called for each new kevent.
Quoted 2 lines
--Mika
Signature · 1 line
Evgeniy Polyakov
Re: [take10 1/2] kevent: Core files.
In reply to Evgeniy Polyakovkernel/kevent/kevent_user.c line 965
Quoted 7 lines
kernel/kevent/kevent_user.c new file +7
-0+static int __devinit kevent_user_init (void ) +{ +int err =0 ; + + err =kevent_sys_init (); +if (err) +panic ("%s: failed to initialize kevent: err=%d.\n" , err);
Here should be?
panic("%s: failed to initialize kevent: err=%d\n",kevent_name, err);
Re: [take10 1/2] kevent: Core files.
Quoted 11 lines · Joe Jin
kernel/kevent/kevent_user.c line 965
Quoted 7 lines
kernel/kevent/kevent_user.c new file +7
-0+static int __devinit kevent_user_init (void ) +{ +int err =0 ; + + err =kevent_sys_init (); +if (err) +panic ("%s: failed to initialize kevent: err=%d.\n" , err);Here should be?
panic("%s: failed to initialize kevent: err=%d\n",kevent_name, err);
The whole function does not exist in the latest patchset anymore.
Signature · 1 line
Evgeniy Polyakov
David Miller
maintainerFrom: Evgeniy Polyakov <[email protected]>
Date: Fri, 28 Jul 2006 09:23:12 +0400
Quoted 3 lines
I completely agree that existing kevent interface is not the best, so I'm opened for any suggestions.
Should kevent creation/removing/modification be separated too?
I do not think so, object for these 3 operations are the same, so there are no typing issues.
Quoted 9 lines · Zach Brown
The easy part is fixing up the somewhat obfuscated collection call. Instead of coming in through a multiplexer that magically treats a void
* as a structkevent_user_controlfollowed by N ukevents (as specified in thekevent_user_control!) we'd turn it into a more explicit collection syscall:int kevent_getevents(int
event_fd, struct ukevent *events,
intmin_events, intmax_events,
struct timeval *timeout);
You've just reinvented io_getevents(). What exactly are we getting from reinventing this (aside from breaking existing apps and creating more of an API mess)?
Quoted 9 lines
Say we have a ring of event structs. AIO has this today, but it sort of gets it wrong because each event element doesn't specify whether it is owned by the kernel or userspace. (It really gets it wrong because it doesn't
flush_dcache_page()after updating the ring viakmap(), but never mind that! No one actually uses thismmap()AIO ring.) In AIO today there is also a control struct mapped along with the ring that has head and tail pointers. We don't want to bounce that cacheline around.
net/socket/af_packet.cgets this right with it'stp_statusmember oftpacket_hdr.
That could be rev'd in the mmap() ring buffer, as there are compat and incompat bits for changing the structure layout. As for bouncing the cacheline of head/tail around, I don't think it matters on real machines, as the multithreaded/SMP case will hit that cacheline bouncing if the user is sharing the event ring between multiple threads on multiple CPUs. The only way around that is to use multiple event rings, say one per node, at which point you have to do load balancing of io requests explicitely between queues (which might be worth it).
Quoted 8 lines
So, great, glibc can now find pending events very quickly if they're waiting in the ring and can fall back to the collection syscall if it wants to wait and the ring is empty. If it consumes events via the syscall it increases its ring index by the number the syscall returned.
There's two things we should address: level events and the notion of only submitting as much as fits in the ring.
Quoted 11 lines
epoll and kevent both have the notion of an event type that always creates an event at the time of the collection syscall while the event source is on a ready list. Think of epoll calling ->poll(POLLOUT) for an empty socket buffer at every
sys_epoll_wait()call. We can't have some source constantly spewing into the ring :/. We could fix this by the API requiring that level events can *only* be collected through the syscall interface. userspace could call into the collection syscall every N events collected through the ring, say. N would be tuned to amortize the syscall cost and still provide fairness or latency for the level sources. I'd be fine with that, especially when it's hidden off in glibc.
This is exactly why I think level triggered events are nasty. It's impossible to do cleanly without requiring a syscall.
Quoted 11 lines
Today AIO only allows submission of as many events as there are space in the ring. It mostly does this so its completion can drop an event in the ring from any context. If we back away from this so that we can have long-lived source registration generate multiple edge events (and I think we want to!), we have to be kind of careful. A source could generate an event while the ring is full. The event could go in a list but if userspace is collecting events in userspace the kernel won't be told when there's space. We'd first have to check this ready list when later events are generated so that pending events on the list aren't overlooked. Userspace would also want to use the collection syscall as the ring empties. Neither seem hard.
As soon as you allow queueing events up in kernel space, it becomes necessary to do another syscall after pulling events out of the queue, which is a waste of CPU cycles when you're under heavy load (exactly the point at which you want the system to be its most efficient). Given that growing the ring buffer is easy enough to do, I'm not sure that the hit is worth it. At some point there has to be some form of flow control involved, and it is much better if it is explicitely obvious where this happens (as opposed to signal queues and our wonderful OOM handling).
-ben
Signature · 2 lines
"Time is of no importance, Mr. President, only life is important." Don't Email: <[email protected]>.
Quoted 6 lines
Quoted 4 lines
int kevent_getevents(int
event_fd, struct ukevent *events,
intmin_events, intmax_events,
struct timeval *timeout);You've just reinvented
io_getevents().
Well, that's certainly one inflammatory way to put it. I would describe it as suggesting that the kevents collection interface not lose the nicer properties of io_getevents().
Quoted 3 lines
What exactly are we getting from
reinventing this (aside from breaking existing apps and creating more of an API mess)?
A generic event collection interface that isn't so strongly bound to the existing semantics of io_setup() and io_submit(). It can be a file descriptor instead of a mysterious cookie/pointer to the mapped region, to start.
Quoted 7 lines
incompat bits for changing the structure layout. As for bouncing the cacheline of head/tail around, I don't think it matters on real machines, as the multithreaded/SMP case will hit that cacheline bouncing if the user is sharing the event ring between multiple threads on multiple CPUs. The only way around that is to use multiple event rings, say one per node, at which point you have to do load balancing of io requests explicitely between queues (which might be worth it).
Sure, so maybe we experiment with these things in the context of the kevent patches and maybe merge them back into the AIO paths if in the end that's the right thing to do. I see no problem with separating development from the existing code.
Quoted 12 lines
Quoted 11 lines
epoll and kevent both have the notion of an event type that always creates an event at the time of the collection syscall while the event source is on a ready list. Think of epoll calling ->poll(POLLOUT) for an empty socket buffer at every
sys_epoll_wait()call. We can't have some source constantly spewing into the ring :/. We could fix this by the API requiring that level events can *only* be collected through the syscall interface. userspace could call into the collection syscall every N events collected through the ring, say. N would be tuned to amortize the syscall cost and still provide fairness or latency for the level sources. I'd be fine with that, especially when it's hidden off in glibc.
Quoted 2 lines
This is exactly why I think level triggered events are nasty. It's impossible to do cleanly without requiring a syscall.
I'm not convinced that it isn't possible to get a sufficiently clean interface that involves the mix.
Quoted 4 lines
As soon as you allow queueing events up in kernel space, it becomes necessary to do another syscall after pulling events out of the queue, which is a waste of CPU cycles when you're under heavy load (exactly the point at which you want the system to be its most efficient).
If we've just consumed a full ring worth of events, and done real work with them, I'm not convinced that an empty syscall is going to be that painful. If we're really under load it might well return some newly arrived events. It becomes a mix of ring completions and syscall completions.
- z
Quoted 10 lines · Zach Brown
Quoted 6 lines
Quoted 4 lines
int kevent_getevents(int
event_fd, struct ukevent *events,
intmin_events, intmax_events,
struct timeval *timeout);You've just reinvented
io_getevents().Well, that's certainly one inflammatory way to put it. I would describe it as suggesting that the kevents collection interface not lose the nicer properties of
io_getevents().
Perhaps, but there seems to be a lot of talk about introducing new APIs where it isn't entirely clear that it is needed. Sorry if that sounded rather acerbic.
Quoted 8 lines
Quoted 3 lines
What exactly are we getting from
reinventing this (aside from breaking existing apps and creating more of an API mess)?A generic event collection interface that isn't so strongly bound to the existing semantics of
io_setup()andio_submit(). It can be a file descriptor instead of a mysterious cookie/pointer to the mapped region, to start.
Things were like that at one point in time, but file descriptors turn out to introduce a huge gaping security hole with SUID programs. The problem is that any event context is closely tied to the address space of the thread issuing the syscalls, and file descriptors do not have this close binding.
Quoted 4 lines
Sure, so maybe we experiment with these things in the context of the kevent patches and maybe merge them back into the AIO paths if in the end that's the right thing to do. I see no problem with separating development from the existing code.
Excellent!
Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
epoll and kevent both have the notion of an event type that always creates an event at the time of the collection syscall while the event source is on a ready list. Think of epoll calling ->poll(POLLOUT) for an empty socket buffer at every
sys_epoll_wait()call. We can't have some source constantly spewing into the ring :/. We could fix this by the API requiring that level events can *only* be collected through the syscall interface. userspace could call into the collection syscall every N events collected through the ring, say. N would be tuned to amortize the syscall cost and still provide fairness or latency for the level sources. I'd be fine with that, especially when it's hidden off in glibc.
Quoted 5 lines
Quoted 2 lines
This is exactly why I think level triggered events are nasty. It's impossible to do cleanly without requiring a syscall.
I'm not convinced that it isn't possible to get a sufficiently clean interface that involves the mix.
My arguement is that this approach introduces a slow path into the heavily loaded server case. If you can show me how to avoid that, I'd be happy to see such an implementation. =-)
Quoted 10 lines
Quoted 4 lines
As soon as you allow queueing events up in kernel space, it becomes necessary to do another syscall after pulling events out of the queue, which is a waste of CPU cycles when you're under heavy load (exactly the point at which you want the system to be its most efficient).
If we've just consumed a full ring worth of events, and done real work with them, I'm not convinced that an empty syscall is going to be that painful. If we're really under load it might well return some newly arrived events. It becomes a mix of ring completions and syscall completions.
Except that you're not usually pulling a full ring worth of events at a time, more often just one. One of the driving forces behind AIO use is in realtime apps where you don't want to eat occasional spikes in the latency of request processing, one just wants to eat the highest priority event then work on the next. By keeping each step small and managable, the properties of the system are much easier to predict. Yes, batching can be helpful performance-wise, but it is somewhat opposite to the design criteria that need to be considered. The right way to cope with that may be to have two different modes of operation that trade off one way or the other on the batching question.
-ben
Signature · 2 lines
"Time is of no importance, Mr. President, only life is important." Don't Email: <[email protected]>.
Quoted 8 lines · Benjamin LaHaise
Quoted 7 lines · Zach Brown
Quoted 6 lines
Quoted 4 lines
int kevent_getevents(int
event_fd, struct ukevent *events,
intmin_events, intmax_events,
struct timeval *timeout);You've just reinvented
io_getevents().
Quoted 7 lines
Quoted 3 lines
Well, that's certainly one inflammatory way to put it. I would describe it as suggesting that the kevents collection interface not lose the nicer properties of
io_getevents().Perhaps, but there seems to be a lot of talk about introducing new APIs where it isn't entirely clear that it is needed. Sorry if that sounded rather acerbic.
Except that kevent has completely different structures and it is possible to have a lot of more types of event than AIO has and there is no need to specially have bytes, offsets and so on.
Magic pointer returned from aio syscall is no pollable too.
Quoted 9 lines
Quoted 8 lines
Quoted 3 lines
What exactly are we getting from
reinventing this (aside from breaking existing apps and creating more of an API mess)?A generic event collection interface that isn't so strongly bound to the existing semantics of
io_setup()andio_submit(). It can be a file descriptor instead of a mysterious cookie/pointer to the mapped region, to start.
Quoted 5 lines
Things were like that at one point in time, but file descriptors turn out to introduce a huge gaping security hole with SUID programs. The problem is that any event context is closely tied to the address space of the thread issuing the syscalls, and file descriptors do not have this close binding.
It is true for all shared resources no matter if it is file descriptor or mapped area.
Quoted 22 lines
Quoted 4 lines
Sure, so maybe we experiment with these things in the context of the kevent patches and maybe merge them back into the AIO paths if in the end that's the right thing to do. I see no problem with separating development from the existing code.
Excellent!
Quoted 15 lines
Quoted 14 lines
Quoted 11 lines
epoll and kevent both have the notion of an event type that always creates an event at the time of the collection syscall while the event source is on a ready list. Think of epoll calling ->poll(POLLOUT) for an empty socket buffer at every
sys_epoll_wait()call. We can't have some source constantly spewing into the ring :/. We could fix this by the API requiring that level events can *only* be collected through the syscall interface. userspace could call into the collection syscall every N events collected through the ring, say. N would be tuned to amortize the syscall cost and still provide fairness or latency for the level sources. I'd be fine with that, especially when it's hidden off in glibc.This is exactly why I think level triggered events are nasty. It's impossible to do cleanly without requiring a syscall.
Quoted 6 lines
Quoted 2 lines
I'm not convinced that it isn't possible to get a sufficiently clean interface that involves the mix.
My arguement is that this approach introduces a slow path into the heavily loaded server case. If you can show me how to avoid that, I'd be happy to see such an implementation. =-)
If I understand you correct you are talking about level triggering events, which arrive continuously? Such events are handled by only one kevent, if iti is ready, that means that at least one such event was fired. One can add a number of fired events as a hints inside kevent.
Quoted 11 lines
Quoted 10 lines
Quoted 4 lines
As soon as you allow queueing events up in kernel space, it becomes necessary to do another syscall after pulling events out of the queue, which is a waste of CPU cycles when you're under heavy load (exactly the point at which you want the system to be its most efficient).
If we've just consumed a full ring worth of events, and done real work with them, I'm not convinced that an empty syscall is going to be that painful. If we're really under load it might well return some newly arrived events. It becomes a mix of ring completions and syscall completions.
Quoted 10 lines
Except that you're not usually pulling a full ring worth of events at a time, more often just one. One of the driving forces behind AIO use is in realtime apps where you don't want to eat occasional spikes in the latency of request processing, one just wants to eat the highest priority event then work on the next. By keeping each step small and managable, the properties of the system are much easier to predict. Yes, batching can be helpful performance-wise, but it is somewhat opposite to the design criteria that need to be considered. The right way to cope with that may be to have two different modes of operation that trade off one way or the other on the batching question.
It is user who should decide either he wants one, all or at least one eventt. kevent supports all types of requests, it's behaviour depends on parameters.
Quoted 1 line
-ben
Signature · 1 line
Evgeniy Polyakov
Quoted 6 lines
Things were like that at one point in time, but file descriptors turn out to introduce a huge gaping security hole with SUID programs. The problem is that any event context is closely tied to the address space of the thread issuing the syscalls, and file descriptors do not have this close binding.
Can you go into that hole in more detail?
Quoted 2 lines
Except that you're not usually pulling a full ring worth of events at a time, more often just one.
OK, but then to wait for it you were already sleeping in the kernel, right?
Clearly we should port httpd to kevents and take some measurements :)
- z
Quoted 1 line · Zach Brown
Clearly we should port httpd to kevents and take some measurements :)
One of my main kevent benchmarks (socket notifications for accept/receive) is handmade http server.
I compared it with FreeBSD kqueue, epoll and kevent_poll (this is generic poll/select notifications ported to kevent) based (it is the same server but with different event functions.
Client was httperf, I ran it with 30k connections in bursts of 3k connection with 1 second timeout between bursts.
Here are results:
kevent: more than 2600 requests/second
epoll and kevent_poll: about 1600-1800 requests/second
kqueue: enormous number of connection reset errors (only 62% of successfull connections) (likely misconfiguration, default FreeBSD 6-something does not allow such rates at all).
More info can be found on kevent homepage:
http://tservice.net.ru/~s0mbre/old/?section=projects&item=kevent
Quoted 1 line
- z
Signature · 1 line
Evgeniy Polyakov
Quoted 5 lines · Evgeniy Polyakov
Quoted 1 line · Zach Brown
Clearly we should port httpd to kevents and take some measurements :)
One of my main kevent benchmarks (socket notifications for accept/receive) is handmade http server.
Yeah, so I noticed. That's a good starting point but I'm more interested in seeing the work integrated with servers that have to survive outside of benchmarking runs.
- z
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Clearly we should port httpd to kevents and take some measurements :)
oh, I see, I forgot the 't' in 'thttpd'. My mistake.
- z
David Miller
maintainerFrom: Zach Brown <[email protected]>
Date: Thu, 27 Jul 2006 12:18:42 -0700
[ I kept this thread around in my inbox because I wanted to give it
some deep thought, so sorry for replying to old bits... ]
Quoted 7 lines
So as the kernel generates events in the ring it only produces an event if the ownership field says that userspace has consumed it and in doing so it sets the ownership field to tell userspace that an event is waiting. userspace and the kernel now each follow their index around the ring as the ownership field lets them produce or consume the event at their index. Can someone tell me if the cache coherence costs of this are extreme? I'm hoping they're not.
No need for an owner field, we can use something like a VJ netchannel datastructure for this. Kernel only writes to producer index and user only writes to consumer index.
Quoted 4 lines
So, great, glibc can now find pending events very quickly if they're waiting in the ring and can fall back to the collection syscall if it wants to wait and the ring is empty. If it consumes events via the syscall it increases its ring index by the number the syscall returned.
I do not think if we do a ring buffer that events should be obtainable via a syscall at all. Rather, I think this system call should be purely "sleep until ring is not empty".
This is actually reasonably simple stuff to implement as Evgeniy has tried to explain.
Events in kevent live on a ready list when they have triggered. Existence on a list determined the state, and I think this design btw invalidates some of the arguments against using netlink that Ulrich mentions in his paper. If netlink socket queuing fails, well then kevent stays on ready list and that is all until the kevent can be successfully published to the user.
I am not advocating netlink at all for this, as the ring buffer idea is much better.
The ring buffer size, as Evgeniy also tried to describe, is bounded purely by the number of registered events. So event loop of application might look something like this:
struct ukevent cur_event;
struct timeval timeo;
setup_timeout(&timeo);
for (;;) {
int err;
while(!(err = ukevent_dequeue(evt_fd, evt_ring,
&cur_event, &timeo))) {
struct my_event_object *o =
event_to_object(&cur_event);
o->dispatch(o, &cur_event);
setup_timeout(&timeo);
}
if (err == -ETIMEDOUT)
timeout_processing();
else
event_error_processing(err);
}ukevent_dequeue() is perhaps some GLIBC implemented routine which does something like:
int err;
for (;;) {
if (!evt_ring_empty(evt_ring)) {
struct ukevent *p = evt_ring_consume(evt_ring);
memcpy(event_p, p, sizeof(struct ukevent));
return 0;
}
err = kevent_wait(evt_fd, timeo_p);
if (err < 0)
break;
}
return err;It's just some stupid ideas... we could also choose to expose the ring buffer layout directly to the user event loop and let it perform the dequeue operation and kevent_wait() calls directly. I don't see why not to allow that.
Quoted 4 lines
I do not think if we do a ring buffer that events should be obtainable via a syscall at all. Rather, I think this system call should be purely "sleep until ring is not empty".
Mmm, yeah, of course. That's much simpler. I'm looking forward to Evgeniy's next patch set.
Quoted 2 lines
The ring buffer size, as Evgeniy also tried to describe, is bounded purely by the number of registered events.
Yeah. fwiw, fs/aio.c has this property today.
- z