Messages in this thread Patch in this message | | | From | Roland Dreier <> | Subject | Re: [PATCH 3/3 v6] IB: expand ib_umem_get() prototype | Date | Tue, 08 Apr 2008 12:40:21 -0700 |
| |
All of this looks good to me, and I'm happy we are finally fixing this up. However, now that it is finally ready to merge, I started thinking about the libmthca changes required to handle this, and I realized that we can actually avoid this:
> --- a/drivers/infiniband/hw/mthca/mthca_user.h > +++ b/drivers/infiniband/hw/mthca/mthca_user.h > @@ -41,7 +41,7 @@ > * Increment this value if any changes that break userspace ABI > * compatibility are made. > */ > -#define MTHCA_UVERBS_ABI_VERSION 1 > +#define MTHCA_UVERBS_ABI_VERSION 2
Instead the mthca kernel driver can look at the size of the data passed in from userspace, and fall back to the old behavior if it's talking to old userspace.
I think this is a net win. The current patch gives the following:
old kernel, old userspace: current behavior (subtle failure on Altix) new kernel, old userspace: refuses to run (on all systems) old kernel, new userspace: current behavior (broken on Altix), new kernel, new userspace: works everywhere
with my proposed change, the only difference is:
new kernel, old userspace: current behavior (subtle failure on Altix), plus print a warning telling people to update userspace.
and given that the vast majority of the systems where mthca is used are *not* Altix systems, it seems more friendly to leave them working across a kernel update.
Andrew: if Arthur agrees with this, please roll the patch below into the current IB patch you have as part of the dma attrs series.
Thanks, Roland
diff --git a/drivers/infiniband/hw/mthca/mthca_provider.c b/drivers/infiniband/hw/mthca/mthca_provider.c index ca5bab0..d2e1429 100644 --- a/drivers/infiniband/hw/mthca/mthca_provider.c +++ b/drivers/infiniband/hw/mthca/mthca_provider.c @@ -367,6 +367,8 @@ static struct ib_ucontext *mthca_alloc_ucontext(struct ib_device *ibdev, return ERR_PTR(-EFAULT); } + context->reg_mr_warned = 0; + return &context->ibucontext; } @@ -1013,7 +1015,15 @@ static struct ib_mr *mthca_reg_user_mr(struct ib_pd *pd, u64 start, u64 length, int err = 0; int write_mtt_size; - if (ib_copy_from_udata(&ucmd, udata, sizeof ucmd)) + if (udata->inlen < sizeof ucm) { + if (!to_mucontext(pd->uobject->context)->reg_mr_warned) { + mthca_warn(dev, "Process %s did not pass in MR attrs\n", + current->comm); + mthca_warn(dev, " Update libmthca to fix this.\n"); + } + ++to_mucontext(pd->uobject->context)->reg_mr_warned; + ucmd.mr_attrs = 0; + } else if (ib_copy_from_udata(&ucmd, udata, sizeof ucmd)) return ERR_PTR(-EFAULT); mr = kmalloc(sizeof *mr, GFP_KERNEL); diff --git a/drivers/infiniband/hw/mthca/mthca_provider.h b/drivers/infiniband/hw/mthca/mthca_provider.h index 262616c..934bf95 100644 --- a/drivers/infiniband/hw/mthca/mthca_provider.h +++ b/drivers/infiniband/hw/mthca/mthca_provider.h @@ -67,6 +67,7 @@ struct mthca_ucontext { struct ib_ucontext ibucontext; struct mthca_uar uar; struct mthca_user_db_table *db_tab; + int reg_mr_warned; }; struct mthca_mtt; diff --git a/drivers/infiniband/hw/mthca/mthca_user.h b/drivers/infiniband/hw/mthca/mthca_user.h index f8cb3b6..e1262c9 100644 --- a/drivers/infiniband/hw/mthca/mthca_user.h +++ b/drivers/infiniband/hw/mthca/mthca_user.h @@ -41,7 +41,7 @@ * Increment this value if any changes that break userspace ABI * compatibility are made. */ -#define MTHCA_UVERBS_ABI_VERSION 2 +#define MTHCA_UVERBS_ABI_VERSION 1 /* * Make sure that all structs defined in this file remain laid out so @@ -62,10 +62,12 @@ struct mthca_alloc_pd_resp { }; struct mthca_reg_mr { +/* + * Mark the memory region with a DMA attribute that causes + * in-flight DMA to be flushed when the region is written to: + */ +#define MTHCA_MR_DMASYNC 0x1 __u32 mr_attrs; -#define MTHCA_MR_DMASYNC 0x1 -/* mark the memory region with a DMA attribute that causes - * in-flight DMA to be flushed when the region is written to */ __u32 reserved; };
| |