On Thu, Dec 11, 2014 at 2:39 PM, Yann Droneaud <[email protected]> wrote: > Le jeudi 11 décembre 2014 à 13:09 +0200, Haggai Eran a écrit : >> On 10/12/2014 18:22, Yann Droneaud wrote: >> > Hi, >> > >> > Le mardi 11 novembre 2014 à 18:36 +0200, Haggai Eran a écrit : >> >> In some drivers there's a need to read data from a user space area that >> >> was pinned using ib_umem, when running from a different process context. >> >> >> >> The ib_umem_copy_from function allows reading data from the physical pages >> >> pinned in the ib_umem struct. >> >> >> >> Signed-off-by: Haggai Eran <[email protected]> >> >> --- >> >> drivers/infiniband/core/umem.c | 26 ++++++++++++++++++++++++++ >> >> include/rdma/ib_umem.h | 2 ++ >> >> 2 files changed, 28 insertions(+) >> >> >> >> diff --git a/drivers/infiniband/core/umem.c >> >> b/drivers/infiniband/core/umem.c >> >> index e0f883292374..77bec75963e7 100644 >> >> --- a/drivers/infiniband/core/umem.c >> >> +++ b/drivers/infiniband/core/umem.c >> >> @@ -292,3 +292,29 @@ int ib_umem_page_count(struct ib_umem *umem) >> >> return n; >> >> } >> >> EXPORT_SYMBOL(ib_umem_page_count); >> >> + >> >> +/* >> >> + * Copy from the given ib_umem's pages to the given buffer. >> >> + * >> >> + * umem - the umem to copy from >> >> + * offset - offset to start copying from >> >> + * dst - destination buffer >> >> + * length - buffer length >> >> + * >> >> + * Returns the number of copied bytes, or an error code. >> >> + */ >> >> +int ib_umem_copy_from(struct ib_umem *umem, size_t offset, void *dst, >> >> + size_t length) >> > >> > I would prefer the arguments in the same order as ib_copy_from_udata() >> > >> > int ib_umem_copy_from(void *dst, >> > struct ib_umem *umem, size_t umem_offset, >> > size_t length); >> >> No problem. >> >> >> +{ >> >> + size_t end = offset + length; >> >> + >> >> + if (offset > umem->length || end > umem->length || end < offset) { >> >> + pr_err("ib_umem_copy_from not in range. offset: %zd umem >> >> length: %zd end: %zd\n", >> >> + offset, umem->length, end); >> >> + return -EINVAL; >> >> + } >> >> + > > I think the test could be rewritten as: > > if (offset > umem->length || length > umem->length - offset) > > That's one operation less. > > >> >> + return sg_pcopy_to_buffer(umem->sg_head.sgl, umem->nmap, dst, length, >> >> + offset + ib_umem_offset(umem)); >> >> +} >> >> +EXPORT_SYMBOL(ib_umem_copy_from); >> > >> > As the function return a "int", no more than INT_MAX bytes (likely 2^31 >> > - 1) can be copied. Perhaps changing the return type to to ssize_t would >> > be better (and a check to enfore ssize_t maximum value). Or change the >> > function could return 0 in case of success or a error code, just like >> > ib_copy_from_udata(). >> > >> >> Okay. I'll change it to match ib_copy_from_udata. We're checking the >> umem size in the call site of this function anyway, and the only reason >> I see sg_pcopy_to_buffer would return less than *length* bytes is when >> reaching the end of the scatterlist. >> > > As the length is compared against umem->length (+ offset), this would > mean umem->length is not "synchronized" with the length of the data > described by the scatter/gather list ?
> BTW, ib_copy_from_udata() is defined as an inline function. Would it be > better to have ib_umem_copy_from() being an inline function too ? > (In such case, I would remove the error message to not duplicate it > across all modules using the function) Yann, Lets leave your 2nd comment to be addressed as future cleanup/improvement. Really, these patches are on the list for 7-8 moths and we were asking for feedback over all this time, jumping now with inlining comments and such when the merge window is just across the door isn't appropriate. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
