lkml.org 
[lkml]   [2015]   [Jul]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 03/20] xen/grant: Introduce helpers to split a page into grant
    On Thu, 9 Jul 2015, Julien Grall wrote:
    > Currently, a grant is always based on the Xen page granularity (i.e
    > 4KB). When Linux is using a different page granularity, a single page
    > will be split between multiple grants.
    >
    > The new helpers will be in charge to split the Linux page into grant and
    ^
    grants

    > call a function given by the caller on each grant.
    >
    > In order to help some PV drivers, the callback is allowed to use less
    > data and must update the resulting length. This is useful for netback.
    >
    > Also provide and helper to count the number of grants within a given
    > contiguous region.
    >
    > Signed-off-by: Julien Grall <julien.grall@citrix.com>
    > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
    > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
    > Cc: David Vrabel <david.vrabel@citrix.com>
    > ---
    > Changes in v2:
    > - Patch added
    > ---
    > drivers/xen/grant-table.c | 26 ++++++++++++++++++++++++++
    > include/xen/grant_table.h | 41 +++++++++++++++++++++++++++++++++++++++++
    > 2 files changed, 67 insertions(+)
    >
    > diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
    > index 62f591f..3679293 100644
    > --- a/drivers/xen/grant-table.c
    > +++ b/drivers/xen/grant-table.c
    > @@ -296,6 +296,32 @@ int gnttab_end_foreign_access_ref(grant_ref_t ref, int readonly)
    > }
    > EXPORT_SYMBOL_GPL(gnttab_end_foreign_access_ref);
    >
    > +void gnttab_foreach_grant(struct page *page, unsigned int offset,
    > + unsigned int len, xen_grant_fn_t fn,
    > + void *data)
    > +{
    > + unsigned int goffset;
    > + unsigned int glen;
    > + unsigned long pfn;

    I would s/pfn/xen_pfn/ inside this function for clarity


    > + len = min_t(unsigned int, PAGE_SIZE - offset, len);
    > + goffset = offset & ~XEN_PAGE_MASK;

    I don't think we want to support cases where (offset & ~XEN_PAGE_MASK)
    != 0, we should just return error.


    > + pfn = xen_page_to_pfn(page) + (offset >> XEN_PAGE_SHIFT);
    > +
    > + while (len) {
    > + glen = min_t(unsigned int, XEN_PAGE_SIZE - goffset, len);

    Similarly I don't think we want to support glen != XEN_PAGE_SIZE here


    > + fn(pfn_to_mfn(pfn), goffset, &glen, data);

    Allowing the callee to change glen makes the interface more complex and
    certainly doesn't match the gnttab_foreach_grant function name anymore.

    If netback needs it, could it just do the work inside its own function?
    I would rather keep gnttab_foreach_grant simple and move the complexity
    there.


    > + goffset += glen;
    > + if (goffset == XEN_PAGE_SIZE) {
    > + goffset = 0;
    > + pfn++;
    > + }

    With the assumptions above you can simplify this


    > + len -= glen;
    > + }
    > +}
    > +
    > struct deferred_entry {
    > struct list_head list;
    > grant_ref_t ref;
    > diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
    > index 4478f4b..6f77378 100644
    > --- a/include/xen/grant_table.h
    > +++ b/include/xen/grant_table.h
    > @@ -45,8 +45,10 @@
    > #include <asm/xen/hypervisor.h>
    >
    > #include <xen/features.h>
    > +#include <xen/page.h>
    > #include <linux/mm_types.h>
    > #include <linux/page-flags.h>
    > +#include <linux/kernel.h>
    >
    > #define GNTTAB_RESERVED_XENSTORE 1
    >
    > @@ -224,4 +226,43 @@ static inline struct xen_page_foreign *xen_page_foreign(struct page *page)
    > #endif
    > }
    >
    > +/* Split Linux page in chunk of the size of the grant and call fn
    > + *
    > + * Parameters of fn:
    > + * mfn: machine frame number based on grant granularity
    > + * offset: offset in the grant
    > + * len: length of the data in the grant. If fn decides to use less data,
    > + * it must update len.
    > + * data: internal information
    > + */
    > +typedef void (*xen_grant_fn_t)(unsigned long mfn, unsigned int offset,
    > + unsigned int *len, void *data);
    > +
    > +void gnttab_foreach_grant(struct page *page, unsigned int offset,
    > + unsigned int len, xen_grant_fn_t fn,
    > + void *data);
    > +
    > +/* Helper to get to call fn only on the first "grant chunk" */
    > +static inline void gnttab_one_grant(struct page *page, unsigned int offset,
    > + unsigned len, xen_grant_fn_t fn,
    > + void *data)
    > +{
    > + /* The first request is limited to the size of one grant */
    > + len = min_t(unsigned int, XEN_PAGE_SIZE - (offset & ~XEN_PAGE_MASK),
    > + len);

    I would just BUG_ON(offset & ~XEN_PAGE_MASK) and simply len = XEN_PAGE_SIZE;


    > + gnttab_foreach_grant(page, offset, len, fn, data);
    > +}
    > +
    > +/* Get the number of grant in a specified region
    > + *
    > + * offset: Offset in the first page

    I would generalize this function and support offset > PAGE_SIZE. At that
    point you could rename offset to "start".


    > + * len: total lenght of data (can cross multiple page)
    > + */
    > +static inline unsigned int gnttab_count_grant(unsigned int offset,
    > + unsigned int len)
    > +{
    > + return (XEN_PFN_UP((offset & ~XEN_PAGE_MASK) + len));
    > +}
    > +
    > #endif /* __ASM_GNTTAB_H__ */




    \
     
     \ /
      Last update: 2015-07-16 17:21    [W:4.100 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site