Messages in this thread | | | Date | Fri, 12 Dec 2003 11:21:11 -0800 | From | William Lee Irwin III <> | Subject | Re: [RFC/PATCH] FUSYN 5/10: kernel fuqueues |
| |
On Wed, Dec 03, 2003 at 12:51:34AM -0800, inaky.perez-gonzalez@intel.com wrote: > +static inline void __debug_outb (unsigned val, int port) { > + __asm__ __volatile__ ("outb %b0,%w1" : : "a" (val), "Nd" (port)); > +} > +static inline unsigned __debug_inb (int port) { > + unsigned value; > + __asm__ __volatile__ ("inb %w1,%b0" : "=a" (value) : "Nd" (port)); > + return value; > +} > +static inline > +void __debug_printstr (const char *str) { > + const int port = 0x03f8; > + while (*str) { > + while (!(__debug_inb (port + UART_LSR) & UART_LSR_THRE)); > + __debug_outb (*str++, port+UART_TX); > + } > + __debug_outb ('\r', port + UART_TX); > +} > +#endif
In general, this kind of debug code shouldn't go in "shipped" patches.
On Wed, Dec 03, 2003 at 12:51:34AM -0800, inaky.perez-gonzalez@intel.com wrote: > +#define assert(c, a...) do { if ((DEBUG >= 0) && !(c)) BUG(); } while (0)
assert() is a no-no in Linux, for various reasons.
On Wed, Dec 03, 2003 at 12:51:34AM -0800, inaky.perez-gonzalez@intel.com wrote: > + * FIXME: is it worth to have get/put? maybe they should be enforced > + * for every fuqueue, this way we don't have to query the ops > + * structure for the get/put method and if it is there, call > + * it. We'd have to move the get/put ops over to the vlocator, > + * but that's not much of a problem. > + * The decission factor is that an atomic operation needs to > + * lock the whole bus and is not as scalable as testing a ptr > + * for NULLness. > + * For simplicity, probably the idea of forcing the refcount in > + * the fuqueue makes sense.
Basically, if there aren't multiple implementations of ->get()/->put() that need to be disambiguated, this should get left out.
On Wed, Dec 03, 2003 at 12:51:34AM -0800, inaky.perez-gonzalez@intel.com wrote: > +#if DEBUG > 0 > +/* BUG_ON() firing? Temporary fix, do you have CONFIG_PREEMPT enabled? > + * either that or disable DEBUG (force #define it to zero). */ > +#define CHECK_IRQs() do { BUG_ON (!in_atomic()); } while (0) > +#else > +#define CHECK_IRQs() do {} while (0) > +#endif
This kind of thing isn't good to have in shipped patches either.
On Wed, Dec 03, 2003 at 12:51:34AM -0800, inaky.perez-gonzalez@intel.com wrote: > +/* Priority-sorted list */ > +struct plist { > + unsigned prio; > + struct list_head node; > +};
Maybe the expectation is for short lists, but it might be better to use an rbtree or other sublinear insertion/removal structure for this. It would be nice if we had a generic heap structure for this sort of affair.
On Wed, Dec 03, 2003 at 12:51:34AM -0800, inaky.perez-gonzalez@intel.com wrote: > +void fuqueue_chprio (struct task_struct *task) > +{ > + struct fuqueue_ops *ops; > + struct fuqueue *fuqueue; > + struct fuqueue_waiter *w; > + int prio = task->prio; > + unsigned long flags; > + > + __ftrace (1, "(%p [%d])\n", task, task->pid); > + CHECK_IRQs(); > + > + local_irq_save (flags); > + preempt_disable(); > + get_task_struct (task); > +next: > + /* Who is the task waiting for? safely acquire and lock it */ > + _raw_spin_lock (&task->fuqueue_wait_lock); > + fuqueue = task->fuqueue_wait; > + if (fuqueue == NULL) /* Ok, not waiting */ > + goto out_task_unlock; > + if (!_raw_spin_trylock (&fuqueue->lock)) { /* Spin dance... */ > + _raw_spin_unlock (&task->fuqueue_wait_lock); > + goto next; > + } > + ops = fuqueue->ops; > + if (ops->get) > + ops->get (fuqueue); > + > + w = task->fuqueue_waiter; > + _raw_spin_unlock (&task->fuqueue_wait_lock); > + put_task_struct (task); > + > + /* Ok, we need to propagate the prio change to the fuqueue */ > + ops = fuqueue->ops; > + task = ops->chprio (task, fuqueue, w); > + if (task == NULL) > + goto out_fuqueue_unlock; > + > + /* Do we need to propagate to the next task */ > + get_task_struct (task); > + _raw_spin_unlock (&fuqueue->lock); > + if (ops->put) > + ops->put (fuqueue); > + ldebug (1, "__set_prio (%d, %d)\n", task->pid, prio); > + __set_prio (task, prio); > + goto next; > + > +out_fuqueue_unlock: > + _raw_spin_unlock (&fuqueue->lock); > + if (ops->put) > + ops->put (fuqueue); > + goto out; > + > +out_task_unlock: > + _raw_spin_unlock (&task->fuqueue_wait_lock); > + put_task_struct (task); > +out: > + local_irq_restore (flags); > + preempt_enable(); > + return;
Heavy use of _raw_spin_lock() like this is a poor practice.
On Wed, Dec 03, 2003 at 12:51:34AM -0800, inaky.perez-gonzalez@intel.com wrote: > +/** Fuqueue operations for usage within the kernel */ > +struct fuqueue_ops fuqueue_ops = { > + .get = NULL, > + .put = NULL, > + .wait_cancel = __fuqueue_wait_cancel, > + .chprio = __fuqueue_chprio > +};
If this is all ->get() and ->put() are going to be, why bother?
-- wli - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |