On Thursday 16 August 2007, Sebastian Siewior wrote:
> +config KSPU
> + bool "Support for utilisation of SPU by the kernel"
> + depends on SPU_FS && EXPERIMENTAL
> + help
> + With this option enabled, the kernel is able to utilize the SPUs for its
> + own tasks.
It might be better to not have this user-selectable at all, but to
autoselect the option when it's used by other code.
> +out_archcoredump:
> + printk("kspu_init() failed\n");
> + unregister_arch_coredump_calls(&spufs_coredump_calls);
> out_syscalls:
> unregister_spu_syscalls(&spufs_calls);
> out_fs:
> @@ -804,12 +811,14 @@ out_sched:
> out_cache:
> kmem_cache_destroy(spufs_inode_cache);
> out:
> + printk("spufs init not performed\n");
> return ret;
> }
The printk lines don't follow the convention of using KERN_*
printk levels. I suggest you either remove them or turn them
into pr_debug().
> +
> +#include <asm/spu_priv1.h>
> +#include <asm/kspu/kspu.h>
> +#include <asm/kspu/merged_code.h>
> +#include <linux/kthread.h>
> +#include <linux/module.h>
> +#include <linux/init_task.h>
> +#include <linux/hardirq.h>
> +#include <linux/kernel.h>
#include lines should be ordered alphabetically, and <asm/ lines come
after <linux/ lines.
> +/*
> + * based on run.c spufs_run_spu
> + */
> +static int spufs_run_kernel_spu(void *priv)
> +{
> + struct kspu_context *kctx = (struct kspu_context *) priv;
> + struct spu_context *ctx = kctx->spu_ctx;
> + int ret;
> + u32 status;
> + unsigned int npc = 0;
> + int fastpath;
> + DEFINE_WAIT(wait_for_stop);
> + DEFINE_WAIT(wait_for_ibox);
> + DEFINE_WAIT(wait_for_newitem);
> +
> + spu_enable_spu(ctx);
> + ctx->event_return = 0;
> + spu_acquire(ctx);
> + if (ctx->state == SPU_STATE_SAVED) {
> + __spu_update_sched_info(ctx);
> +
> + ret = spu_activate(ctx, 0);
> + if (ret) {
> + spu_release(ctx);
> + printk(KERN_ERR "could not obtain runnable spu: %d\n",
> + ret);
> + BUG();
> + }
> + } else {
> + /*
> + * We have to update the scheduling priority under active_mutex
> + * to protect against find_victim().
> + */
> + spu_update_sched_info(ctx);
> + }
The code you have copied this from has recently been changed to also set an initial
time slice, you should do the same change here.
> +
> + spu_run_init(ctx, &npc);
> + do {
> + fastpath = 0;
> + prepare_to_wait(&ctx->stop_wq, &wait_for_stop,
> + TASK_INTERRUPTIBLE);
> + prepare_to_wait(&ctx->ibox_wq, &wait_for_ibox,
> + TASK_INTERRUPTIBLE);
> + prepare_to_wait(&kctx->newitem_wq, &wait_for_newitem,
> + TASK_INTERRUPTIBLE);
> +
> + if (unlikely(test_and_clear_bit(SPU_SCHED_NOTIFY_ACTIVE,
> + &ctx->sched_flags))) {
> +
> + if (!(status & SPU_STATUS_STOPPED_BY_STOP)) {
> + spu_switch_notify(ctx->spu, ctx);
> + }
> + }
> +
> + spuctx_switch_state(ctx, SPU_UTIL_SYSTEM);
> +
> + pr_debug("going to handle class1\n");
> + ret = spufs_handle_class1(ctx);
> + if (unlikely(ret)) {
> + /*
> + * SPE_EVENT_SPE_DATA_STORAGE => refernce invalid memory
> + */
> + printk(KERN_ERR "Invalid memory dereferenced by the"
> + "spu: %d\n", ret);
> + BUG();
> + }
> +
> + /* FIXME BUG: We need a physical SPU to discover
> + * ctx->spu->class_0_pending. It is not saved on context
> + * switch. We may lose this on context switch.
> + */
> + status = ctx->ops->status_read(ctx);
> + if (unlikely((ctx->spu && ctx->spu->class_0_pending) ||
> + status & SPU_STATUS_INVALID_INSTR)) {
> + printk(KERN_ERR "kspu error, status_register: 0x%08x\n",
> + status);
> + printk(KERN_ERR "event return: 0x%08lx, spu's npc: "
> + "0x%08x\n", kctx->spu_ctx->event_return,
> + kctx->spu_ctx->ops->npc_read(
> + kctx->spu_ctx));
> + printk(KERN_ERR "class_0_pending: 0x%lx\n", ctx->spu->class_0_pending);
> + print_kctx_debug(kctx);
> + BUG();
> + }
> +
> + if (notify_done_reqs(kctx))
> + fastpath = 1;
> +
> + if (queue_requests(kctx))
> + fastpath = 1;
> +
> + if (!(status & SPU_STATUS_RUNNING)) {
> + /* spu is currently not running */
> + pr_debug("SPU not running, last stop code was: %08x\n",
> + status >> SPU_STOP_STATUS_SHIFT);
> + if (pending_spu_work(kctx)) {
> + /* spu should run again */
> + pr_debug("Activate SPU\n");
> + kspu_fill_dummy_reqs(kctx);
> +
> + spu_run_fini(ctx, &npc, &status);
> + spu_acquire_runnable(ctx, 0);
> + spu_run_init(ctx, &npc);
> + } else {
> + /* spu finished work */
> + pr_debug("SPU will remain in stop state\n");
> + spu_run_fini(ctx, &npc, &status);
> + spu_yield(ctx);
> + spu_acquire(ctx);
> + }
> + } else {
> + pr_debug("SPU is running, switch state to util user\n");
> + spuctx_switch_state(ctx, SPU_UTIL_USER);
> + }
> +
> + if (fastpath)
> + continue;
> +
> + spu_release(ctx);
> + schedule();
> + spu_acquire(ctx);
> +
> + } while (!kthread_should_stop() || !list_empty(&kctx->work_queue));
The inner loop is rather long, in an already long function. Can you split out
parts into separate functions here?
> +#endif
> --- a/arch/powerpc/platforms/cell/spufs/spufs.h
> +++ b/arch/powerpc/platforms/cell/spufs/spufs.h
> @@ -344,4 +344,18 @@ static inline void spuctx_switch_state(s
> }
> }
>
> +#ifdef CONFIG_KSPU
> +int __init kspu_init(void);
> +void __exit kspu_exit(void);
The __init and __exit specifiers are not meaningful in the declaration,
you only need them in the definition.
Arnd <><
-
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
