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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel