Re: [PATCH 2.6.31] ehca: Tolerate dynamic memory operations and huge pages

Roland Dreier
2009-06-13T04:51:10Z

OK, one major issue with this patch and a few minor nits.

First, the major issue is that I don't see anything in the patch that
changes the code in ehca_mem_notifier() in ehca_main.c:

case MEM_GOING_ONLINE:
case MEM_GOING_OFFLINE:
/* only ok if no hca is attached to the lpar */
spin_lock_irqsave(&shca_list_lock, flags);
if (list_empty(&shca_list)) {
spin_unlock_irqrestore(&shca_list_lock, flags);
return NOTIFY_OK;
} else {
spin_unlock_irqrestore(&shca_list_lock, flags);
if (printk_timed_ratelimit(&ehca_dmem_warn_time,
30 * 1000))
ehca_gen_err("DMEM operations are not allowed"
"in conjunction with eHCA");
return NOTIFY_BAD;
}

But your patch description says:

> This patch implements toleration of dynamic memory operations....

But it seems you're still going to hit the same NOTIFY_BAD case above
after your patch. So something doesn't compute for me. Could you
explain more?

Second, a nit:

> +#define EHCA_REG_MR 0
> +#define EHCA_REG_BUSMAP_MR (~0)

and you pass these as the reg_busmap parm in:

> int ehca_reg_mr(struct ehca_shca *shca,
> struct ehca_mr *e_mr,
> u64 *iova_start,
> @@ -991,7 +1031,8 @@
> struct ehca_pd *e_pd,
> struct ehca_mr_pginfo *pginfo,
> u32 *lkey, /*OUT*/
> - u32 *rkey) /*OUT*/
> + u32 *rkey, /*OUT*/
> + int reg_busmap)

and test it as:

> + if (reg_busmap)
> + ret = ehca_reg_bmap_mr_rpages(shca, e_mr, pginfo);
> + else
> + ret = ehca_reg_mr_rpages(shca, e_mr, pginfo);

So the ~0 for true looks a bit odd. One option would be to make
reg_busmap a bool, since that's how you're using it, but then you lose
the nice self-documenting macro where you call things.

So I think it would be cleaner to do something like

enum ehca_reg_type {
EHCA_REG_MR,
EHCA_REG_BUSMAP_MR
};

and make the "int reg_busmap" parameter into "enum ehca_reg_type reg_type"
and have the code become

+ if (reg_type == EHCA_REG_BUSMAP_MR)
+ ret = ehca_reg_bmap_mr_rpages(shca, e_mr, pginfo);
+ else if (reg_type == EHCA_REG_MR)
+ ret = ehca_reg_mr_rpages(shca, e_mr, pginfo);
+ else
+ ret = -EINVAL

or something like that.

> +struct ib_dma_mapping_ops ehca_dma_mapping_ops = {
> + .mapping_error = ehca_dma_mapping_error,
> + .map_single = ehca_dma_map_single,
> + .unmap_single = ehca_dma_unmap_single,
> + .map_page = ehca_dma_map_page,
> + .unmap_page = ehca_dma_unmap_page,
> + .map_sg = ehca_dma_map_sg,
> + .unmap_sg = ehca_dma_unmap_sg,
> + .dma_address = ehca_dma_address,
> + .dma_len = ehca_dma_len,
> + .sync_single_for_cpu = ehca_dma_sync_single_for_cpu,
> + .sync_single_for_device = ehca_dma_sync_single_for_device,
> + .alloc_coherent = ehca_dma_alloc_coherent,
> + .free_coherent = ehca_dma_free_coherent,
> +};

I always think structures like this are easier to read if you align the
'=' signs. But no big deal.

> + ret = ehca_create_busmap();
> + if (ret) {
> + ehca_gen_err("Cannot create busmap.");
> + goto module_init2;
> + }
> +
> ret = ibmebus_register_driver(&ehca_driver);
> if (ret) {
> ehca_gen_err("Cannot register eHCA device driver");
> ret = -EINVAL;
> - goto module_init2;
> + goto module_init3;
> }
>
> ret = register_memory_notifier(&ehca_mem_nb);
> if (ret) {
> ehca_gen_err("Failed registering memory add/remove notifier");
> - goto module_init3;
> + goto module_init4;

Having to renumber unrelated things is when something changes is why I
don't like this style of error path labels. But I think it's well and
truly too late to fix that in ehca.

- R.