Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1185939 > unrolled thread

Re: [PATCH v2 03/20] xen/grant: Introduce helpers to split a page into grant

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 03/20] xen/grant: Introduce helpers to split a page  into grant Stefano Stabellini <stefano.stabellini@eu.citrix.com> - 2015-07-16 17:10 +0200
    Re: [PATCH v2 03/20] xen/grant: Introduce helpers to split a page  into grant Julien Grall <julien.grall@citrix.com> - 2015-07-16 18:10 +0200
      Re: [Xen-devel] [PATCH v2 03/20] xen/grant: Introduce helpers to  split a page into grant Julien Grall <julien.grall@citrix.com> - 2015-07-17 15:20 +0200

#1185939 — Re: [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__ */


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1186042

Hi Stefano,

On 16/07/2015 17:01, Stefano Stabellini wrote:
> 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

Will fix it.

>> 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

Ok.

>
>
>> +	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.

We have to support offset != 0. The buffer received by the PV drivers 
may start in the middle of the page and finish before the end of the page.

For instance blkfront is using biovec which contains 3 informations: the 
page, the offset in the page and the total length.

>
>> +	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

See my answer above.

>
>
>> +		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.

Why? Each time the callback is called, there is a new grant allocated.

> 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.

Moving the complexity in netback means adding a loop in the callback
which will do exactly the same as this loop.

That also means to use XEN_PAGE_SIZE & co which I'm trying to avoid in 
order to not confuse the developer. If they are hidden it likely mean 
less problem on 64KB when the developper is working on 4KB.

IHMO, the complexity is not so bad and will be more lisible than yet 
another loop.

[...]


>> +/* 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;

See my remark above.

>
>
>> +	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".

It's actually supported, maybe it's not clear enough.

Regards,

-- 
Julien Grall
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1186813 — Re: [Xen-devel] [PATCH v2 03/20] xen/grant: Introduce helpers to split a page into grant

On 16/07/15 17:07, Julien Grall wrote:
>>> +    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
> 
> See my answer above.
> 
>>
>>
>>> +        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.
> 
> Why? Each time the callback is called, there is a new grant allocated.

As discussed IRL, I will rename this function into
gnttab_foreach_grant_in_range.

> 
>> 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.
> 
> Moving the complexity in netback means adding a loop in the callback
> which will do exactly the same as this loop.
> 
> That also means to use XEN_PAGE_SIZE & co which I'm trying to avoid in
> order to not confuse the developer. If they are hidden it likely mean
> less problem on 64KB when the developper is working on 4KB.
> 
> IHMO, the complexity is not so bad and will be more lisible than yet
> another loop.

After the IRL talk, I will give a look if I can move the "netback
problem" in a different helper. We can see later if we could improve the
code.

Regards,

-- 
Julien Grall
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web