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
signature.asc
Description: This is a digitally signed message part
