Re: [RFC PATCH v01] Xen PVSCSI drivers for pvops xen/stable-2.6.32.x kernel

Re: [RFC PATCH v01] Xen PVSCSI drivers for pvops xen/stable-2.6.32.x kernel

[Nathanael Rensen]

On 7 January 2011 06:35,  Pasi Kärkkäinen <pasik@iki.fi> wrote:
> Hello,
>
> http://pasik.reaktio.net/xen/patches/xen-pvscsi-drivers-linux-2.6.32.27-pvops-v01.diff
>
> This is the first version of Xen PVSCSI drivers, both the scsiback backend and
> scsifront frontend, ported from Novell SLES11SP1 2.6.32 Xenlinux kernel to
> pvops xen/stable-2.6.32.x branch.
>
> -- Pasi

That's great. Some comments based on my experience with pvusb:

1) You've added GNTST_eagain into include/xen/interface/grant_table.h.
When porting the pvusb drivers I removed the "check and retry" loops
based on GNTST_eagain because I noticed that netback and blkback don't
do that. Looking at the grant_table code in xen-4.0-testing I don't
think GNTST_eagain is used now. Perhaps someone with knowledge of the
history of GNTST_eagain could help to clarify.

2) When porting pvusb I found I had to take care with the
gnttab_grant_foreign_access_ref calls. Following the example of
blkback I replaced
   buffer_pfn = page_to_phys(page) >> PAGE_SHIFT;
with
   buffer_mfn = pfn_to_mfn(page_to_pfn(page));
The situation for pvscsi looks a bit more involved (e.g. buffer_pfn++).

3) The existing xen-netfront, xen-blkfront and xen-pcifront have been
converted to a single .c module when ported to pvops. I followed that
lead with pvusb. You might like to do the same for consistency.

Nathanael

Re: [RFC PATCH v01] Xen PVSCSI drivers for pvops xen/stable-2.6.32.x kernel

[Ian Campbell]

On Fri, 2011-01-07 at 10:03 +0000, Nathanael Rensen wrote:
> On 7 January 2011 06:35,  Pasi Kärkkäinen <pasik@iki.fi> wrote:
> > Hello,
> >
> > http://pasik.reaktio.net/xen/patches/xen-pvscsi-drivers-linux-2.6.32.27-pvops-v01.diff
> >
> > This is the first version of Xen PVSCSI drivers, both the scsiback backend and
> > scsifront frontend, ported from Novell SLES11SP1 2.6.32 Xenlinux kernel to
> > pvops xen/stable-2.6.32.x branch.
> >
> > -- Pasi
> 
> That's great. Some comments based on my experience with pvusb:
> 
> 1) You've added GNTST_eagain into include/xen/interface/grant_table.h.
> When porting the pvusb drivers I removed the "check and retry" loops
> based on GNTST_eagain because I noticed that netback and blkback don't
> do that. Looking at the grant_table code in xen-4.0-testing I don't
> think GNTST_eagain is used now. Perhaps someone with knowledge of the
> history of GNTST_eagain could help to clarify.

IIRC GNTST_eagain was added to the classic Xen trees but never forward
ported to pvops.

It's part of the page sharing paging work (so dom0 can deal with a
paged/shared guest rather than for paging of dom0 itself). I guess those
don't work with pvops kernels? CC'ing the guys who are working on that
stuff.

(The presence of the use of msleep in the eagain case is a bit
terrifying. Stripping it out for now and redoing it might not be a bad
plan)

Ian.

Re: [RFC PATCH v01] Xen PVSCSI drivers for pvops xen/stable-2.6.32.x kernel

[Pasi Kärkkäinen]

On Fri, Jan 07, 2011 at 10:26:56AM +0000, Ian Campbell wrote:
> On Fri, 2011-01-07 at 10:03 +0000, Nathanael Rensen wrote:
> > On 7 January 2011 06:35,  Pasi Kärkkäinen <pasik@iki.fi> wrote:
> > > Hello,
> > >
> > > http://pasik.reaktio.net/xen/patches/xen-pvscsi-drivers-linux-2.6.32.27-pvops-v01.diff
> > >
> > > This is the first version of Xen PVSCSI drivers, both the scsiback backend and
> > > scsifront frontend, ported from Novell SLES11SP1 2.6.32 Xenlinux kernel to
> > > pvops xen/stable-2.6.32.x branch.
> > >
> > > -- Pasi
> > 
> > That's great. Some comments based on my experience with pvusb:
> > 
> > 1) You've added GNTST_eagain into include/xen/interface/grant_table.h.
> > When porting the pvusb drivers I removed the "check and retry" loops
> > based on GNTST_eagain because I noticed that netback and blkback don't
> > do that. Looking at the grant_table code in xen-4.0-testing I don't
> > think GNTST_eagain is used now. Perhaps someone with knowledge of the
> > history of GNTST_eagain could help to clarify.
> 
> IIRC GNTST_eagain was added to the classic Xen trees but never forward
> ported to pvops.
> 

Yeah, I noticed GNTST_eagain was missing from pvops, so I just blindly 
copied it from Xenlinux to pvops.. 

With the first version of PVSCSI for pvops I tried to do minimal
changes, just enough to make it compile.. 

So that stuff needs some more attention.

> It's part of the page sharing paging work (so dom0 can deal with a
> paged/shared guest rather than for paging of dom0 itself). I guess those
> don't work with pvops kernels? CC'ing the guys who are working on that
> stuff.
> 
> (The presence of the use of msleep in the eagain case is a bit
> terrifying. Stripping it out for now and redoing it might not be a bad
> plan)
> 

-- Pasi

Re: [RFC PATCH v01] Xen PVSCSI drivers for pvops xen/stable-2.6.32.x kernel

[Olaf Hering]

On Fri, Jan 07, Ian Campbell wrote:

> On Fri, 2011-01-07 at 10:03 +0000, Nathanael Rensen wrote:
> > On 7 January 2011 06:35,  Pasi Kärkkäinen <pasik@iki.fi> wrote:
> > > Hello,
> > >
> > > http://pasik.reaktio.net/xen/patches/xen-pvscsi-drivers-linux-2.6.32.27-pvops-v01.diff
> > >
> > > This is the first version of Xen PVSCSI drivers, both the scsiback backend and
> > > scsifront frontend, ported from Novell SLES11SP1 2.6.32 Xenlinux kernel to
> > > pvops xen/stable-2.6.32.x branch.
> > >
> > > -- Pasi
> > 
> > That's great. Some comments based on my experience with pvusb:
> > 
> > 1) You've added GNTST_eagain into include/xen/interface/grant_table.h.
> > When porting the pvusb drivers I removed the "check and retry" loops
> > based on GNTST_eagain because I noticed that netback and blkback don't
> > do that. Looking at the grant_table code in xen-4.0-testing I don't
> > think GNTST_eagain is used now. Perhaps someone with knowledge of the
> > history of GNTST_eagain could help to clarify.
> 
> IIRC GNTST_eagain was added to the classic Xen trees but never forward
> ported to pvops.
> 
> It's part of the page sharing paging work (so dom0 can deal with a
> paged/shared guest rather than for paging of dom0 itself). I guess those
> don't work with pvops kernels? CC'ing the guys who are working on that
> stuff.

I have not tried the pvops kernel with xenpaging.

> (The presence of the use of msleep in the eagain case is a bit
> terrifying. Stripping it out for now and redoing it might not be a bad
> plan)

Right now the retry is required for xenpaging, otherwise the guests
filesystem will be corrupted.
In the long run the newly added waitqueue feature may be used to make
the page access transparent to the guest and dom0.


Olaf

Re: [RFC PATCH v01] Xen PVSCSI drivers for pvops xen/stable-2.6.32.x kernel

[Ian Campbell]

On Mon, 2011-01-10 at 10:15 +0000, Olaf Hering wrote:
> 
> > (The presence of the use of msleep in the eagain case is a bit
> > terrifying. Stripping it out for now and redoing it might not be a
> bad
> > plan)
> 
> Right now the retry is required for xenpaging, otherwise the guests
> filesystem will be corrupted.
> In the long run the newly added waitqueue feature may be used to make
> the page access transparent to the guest and dom0.

I mistakenly thought msleep was a spinning type wait, but it does
actually sleep properly so it's probably not as bad as I thought.

Ian.

Re: [RFC PATCH v01] Xen PVSCSI drivers for pvops xen/stable-2.6.32.x kernel

[Pasi Kärkkäinen]

On Fri, Jan 07, 2011 at 06:03:59PM +0800, Nathanael Rensen wrote:
> On 7 January 2011 06:35,  Pasi Kärkkäinen <pasik@iki.fi> wrote:
> > Hello,
> >
> > http://pasik.reaktio.net/xen/patches/xen-pvscsi-drivers-linux-2.6.32.27-pvops-v01.diff
> >
> > This is the first version of Xen PVSCSI drivers, both the scsiback backend and
> > scsifront frontend, ported from Novell SLES11SP1 2.6.32 Xenlinux kernel to
> > pvops xen/stable-2.6.32.x branch.
> >
> > -- Pasi
> 
> That's great. Some comments based on my experience with pvusb:
> 
> 1) You've added GNTST_eagain into include/xen/interface/grant_table.h.
> When porting the pvusb drivers I removed the "check and retry" loops
> based on GNTST_eagain because I noticed that netback and blkback don't
> do that. Looking at the grant_table code in xen-4.0-testing I don't
> think GNTST_eagain is used now. Perhaps someone with knowledge of the
> history of GNTST_eagain could help to clarify.
> 

Yep, I commented about this on the other email.

> 2) When porting pvusb I found I had to take care with the
> gnttab_grant_foreign_access_ref calls. Following the example of
> blkback I replaced
>    buffer_pfn = page_to_phys(page) >> PAGE_SHIFT;
> with
>    buffer_mfn = pfn_to_mfn(page_to_pfn(page));
> The situation for pvscsi looks a bit more involved (e.g. buffer_pfn++).
> 

Yeah, I had to hack that stuff aswell.. I was pretty tired so not sure
at all I did it correctly :)

I'll get back to it some day now and take another look at it.
And actually do some *testing* with the patch :)

> 3) The existing xen-netfront, xen-blkfront and xen-pcifront have been
> converted to a single .c module when ported to pvops. I followed that
> lead with pvusb. You might like to do the same for consistency.
> 

Yep, I noticed this aswell. The first version was meant to be
"minimal changes just to make it compile". I need to do this change aswell.

Thanks for the comments! 

-- Pasi

Re: Re: [RFC PATCH v01] Xen PVSCSI drivers for pvops xen/stable-2.6.32.x kernel

[Konrad Rzeszutek Wilk]

> Yep, I noticed this aswell. The first version was meant to be
> "minimal changes just to make it compile". I need to do this change aswell.

I like the idea of keeping the authorship, and the subsequent patches in sync.
If you can please CC me on the patches and I can track them in my git tree.

Should I use the patch you posted or wait a bit?

Re: Re: [RFC PATCH v01] Xen PVSCSI drivers for pvops xen/stable-2.6.32.x kernel

[Pasi Kärkkäinen]

On Fri, Jan 07, 2011 at 11:50:26AM -0500, Konrad Rzeszutek Wilk wrote:
> > Yep, I noticed this aswell. The first version was meant to be
> > "minimal changes just to make it compile". I need to do this change aswell.
> 
> I like the idea of keeping the authorship, and the subsequent patches in sync.
> If you can please CC me on the patches and I can track them in my git tree.
> 

Ok.

> Should I use the patch you posted or wait a bit?
>

I think you should wait a bit.. I'll take another look at it,
and post second version.

-- Pasi