On Thu, 2005-03-31 at 16:27 -0800, Andrew Morton wrote: > Evgeniy Polyakov <[email protected]> wrote: > > > > I'm pleased to annouce CBUS - ultra fast (for insert operations) > > message bus. > > > +static int cbus_enqueue(struct cbus_event_container *c, struct cn_msg *msg) > > +{ > > + int err; > > + struct cbus_event *event; > > + unsigned long flags; > > + > > + event = kmalloc(sizeof(*event) + msg->len, GFP_ATOMIC); > > Using GFP_ATOMIC is a bit lame. It would be better to require the caller > to pass in the gfp_flags. Or simply require that all callers not hold > locks and use GFP_KERNEL. New API with GFP flags provided is a next step in connector's TODO list. > > +static int cbus_process(struct cbus_event_container *c, int all) > > +{ > > + struct cbus_event *event; > > + int len, i, num; > > + > > + if (list_empty(&c->event_list)) > > + return 0; > > + > > + if (all) > > + len = c->qlen; > > + else > > + len = 1; > > + > > + num = 0; > > + for (i=0; i<len; ++i) { > > + event = cbus_dequeue(c); > > + if (!event) > > + continue; > > + > > + cn_netlink_send(&event->msg, 0); > > + num++; > > + > > + kfree(event); > > + } > > + > > + return num; > > +} > > It might be cleaner to pass in an item count rather than a boolean `all' > here. Then again, it seems racy. It was called somehow like we_are_at_the_end_and_must_process_all_events_remain, so cbus_process() could be called from the ->exit() routing. So I decided to call it that way, but I'm not so impracticabile about it. > The initial list_empty() call could fail to detect new events due to lack > of locking and memory barriers. It is perfectly normal, and locking does not exist here for performance reasons. cbus_process() is too low priority in comparison with insert operation, so it can easily miss one entry and process it next time. > We conventionally code for loops as > > for (i = 0; i < len; i++) Grrr.... > > +static int cbus_event_thread(void *data) > > +{ > > + int i, non_empty = 0, empty = 0; > > + struct cbus_event_container *c; > > + > > + daemonize(cbus_name); > > + allow_signal(SIGTERM); > > + set_user_nice(current, 19); > > Please use the kthread api for managing this thread. > > Is a new kernel thread needed? Logic behind cbus is following: 1. make insert operation return as soon as possible, 2. deferring actual message delivering to the safe time That thread does second point. > > + while (!cbus_need_exit) { > > + if (empty || non_empty == 0 || non_empty > 10) { > > + interruptible_sleep_on_timeout(&cbus_wait_queue, 10); > > interruptible_sleep_on_timeout() is heavily deprecated and is racy without > external locking (it pretty much has to be the BKL). Use wait_event_timeout(). Ok. > > +int __devinit cbus_init(void) > > +{ > > + int i, err = 0; > > + struct cbus_event_container *c; > > + > > + for_each_cpu(i) { > > + c = &per_cpu(cbus_event_list, i); > > + cbus_init_event_container(c); > > + } > > + > > + init_completion(&cbus_thread_exited); > > + > > + cbus_pid = kernel_thread(cbus_event_thread, NULL, CLONE_FS | CLONE_FILES); > > Using the kthread API would clean this up. > > > + if (IS_ERR((void *)cbus_pid)) { > > The weird cast here might not even work at all on 64-bit architectures. It > depends if they sign extend ints when casting them to pointers. I guess > they do. If cbus_pid is indeed an s32. > > Much better to do > > if (cbus_pid < 0) I will do it after above issues resolved. > > +void __devexit cbus_fini(void) > > +{ > > + int i; > > + struct cbus_event_container *c; > > + > > + cbus_need_exit = 1; > > + kill_proc(cbus_pid, SIGTERM, 0); > > + wait_for_completion(&cbus_thread_exited); > > + > > + for_each_cpu(i) { > > + c = &per_cpu(cbus_event_list, i); > > + cbus_fini_event_container(c); > > + } > > +} > > I think this is racy. What stops new events from being queued while this > function is in progress? cbus_insert() should check need_exit flag - patch exists, but against my tree, so I wait untill CBUS showed in public, so I can resync with it. -- Evgeniy Polyakov Crash is better than data corruption -- Arthur Grabowski
Attachment:
signature.asc
Description: This is a digitally signed message part
- Follow-Ups:
- References:
- Prev by Date: Re: [2.6 patch] drivers/isdn/i4l/isdn_ppp.c: fix off by one errors
- Next by Date: RE: 2.6.11, USB: High latency?
- Previous by thread: Re: [1/1] CBUS: new very fast (for insert operations) message bus based on kenel connector.
- Next by thread: Re: [1/1] CBUS: new very fast (for insert operations) message bus based on kenel connector.
- Index(es):
 |