Re: [PATCH v3 18/20] net/xen-netback: Make it running on 64KB page granularity

[Julien Grall]

Hi Wei,

On 08/08/2015 15:55, Wei Liu wrote:
> >   struct xenvif_rx_meta {
> >   	int id;
> > @@ -80,16 +81,18 @@ struct xenvif_rx_meta {
> >   /* Discriminate from any valid pending_idx value. */
> >   #define INVALID_PENDING_IDX 0xFFFF
> >
> > -#define MAX_BUFFER_OFFSET PAGE_SIZE
> > +#define MAX_BUFFER_OFFSET XEN_PAGE_SIZE
> >
> >   #define MAX_PENDING_REQS XEN_NETIF_TX_RING_SIZE
> >
> > +#define MAX_XEN_SKB_FRAGS (65536 / XEN_PAGE_SIZE + 1)
> > +
>
> It might be clearer if you add a comment saying the maximum number of
> frags is derived from the page size of the grant page, which happens to
> be XEN_PAGE_SIZE at the moment.

Will do.


> In the future we need to figure out the page size of grant page in a
> dynamic way. We shall cross the bridge when we get there.

Right, there is few other places where we would need to do that too (see 
MAX_BUFFER_OFFSET for instance).


[..]

> > +		info.page = page;
> > +		gnttab_foreach_grant_in_range(page, offset, bytes,
> > +					      xenvif_gop_frag_copy_grant,
> > +					      &info);
>
> Looks like I need to at least wait until the API is settle before giving
> my ack.
>
> >   		size -= bytes;
> > +		offset = 0;
>
> This looks wrong. Should be offset += bytes.

With the new implementation of the loop, each iteration will be on a 
different page.
So only the first page has an offset different than zero.

> > -		/* Next frame */
> > -		if (offset == PAGE_SIZE && size) {
> > +		/* Next page */
> > +		if (size) {
> >   			BUG_ON(!PageCompound(page));
> >   			page++;
> > -			offset = 0;
>
> And this should not be deleted, I think.
>
> What is the reason for changing offset calculation? I think there is
> still compound page when using 64K page.

The compound pages are still working ... gnttab_foreach_grant_in_range 
is called once per page. So the offset can be reset to 0 every time. No 
need to add code which would make the result less clear.

We only need to know if the size is not 0 to get the next page.

The patch may not be clear enough to see it's working so I've copied the 
result loop below:

        while (size > 0) {
                BUG_ON(offset >= PAGE_SIZE);

                bytes = PAGE_SIZE - offset;
                if (bytes > size)
                        bytes = size;

                info.page = page;
                gnttab_foreach_grant_in_range(page, offset, bytes,
                                             xenvif_gop_frag_copy_grant,
                                              &info);
                size -= bytes;
                offset = 0;

                /* Next page */
                if (size) {
                        BUG_ON(!PageCompound(page));
                        page++;
                }
        }

Regards,

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