[RFC,v1,00/10] vfio-ccw channel program rework
mbox series

Message ID 20181109023937.96105-1-farman@linux.ibm.com
Headers show
Series
  • vfio-ccw channel program rework
Related show

Message

Eric Farman Nov. 9, 2018, 2:39 a.m. UTC
Hi Connie, Pierre,

This is a first round of rework to vfio-ccw that I've been cooking to
make the channel program handler easier to understand.  My apologies
for the number of patches; as there are some intersections within them
I couldn't break them up without forcing them to be applied in a
particular order.  I imagine that since there will probably need to be
some discussion, an RFC tag makes sense and one giant series will be
fine for now.  I can break them up for later versions if desired.

(Sidebar: I'm leaving next Thursday for an early start on the US
Thanksgiving holiday.  So if you are busy and can't get to this,
no problem!  Now where did I leave Pierre's rework patches...  :)

Patch summary:

1-2:	Fixes for cleanup on error paths
3:	Code simplication
4-6:	Simplify the ccwchain processing
7-10:	Simplify the pfn_array processing

The first few patches are very straightforward, if unlikely to occur.
They could even be considered candidates for 4.20, if so inclined.

The ccwchain patches are the result of two observations about how we
currently process a channel program and its component CCWs.  One is a
duplicate set of code for a Transfer In Channel (TIC) CCW versus any
other type, which provides an easy opportunity for simplification when
all it's trying to handle is a memory jump.  The other is a duplication
between a direct-addressed CCW and an Indirect Data Address (IDA) CCW.
There is currently the nuance that the direct CCW path creates:

  ch_pat->pfn_array_table[1]->pfn_array[#pages]

while an IDA CCW creates:

  ch_pat->pfn_array_table[#idaws]->pfn_array[1]

It might not be obvious, but #pages is calculated identically to #idaws,
as highlighted by patch 7.  By removing these duplications, we set the
stage for the next patches, which can eliminate an array entirely.

The pfn_array patches permit a non-contiguous array of addresses to be
passed to pfn_array_alloc(_pin), which in turn permits the removal of
struct pfn_array_table and a squashing of our arrays.  Thus, instead of
either of the two nested arrays described above, we have:

  ch_pa->pfn_array[#idaws]

(Doesn't that look nice?!)

The last two patches in this series should be squashed together; I just
left them apart in the RFC to make it easier to see how turn-key this
becomes after the complexities of the previous patches, and without the
noise of realigning the code afterward.  (Sorry, kbuild robot.)

While this series provides no functionality changes, I find that
sizeof(vfio_ccw.ko) decreases by about 7 percent according to the
bloat-o-meter, and we remove some of the malloc load for a given
channel program.  Hopefully there's also an increase in Readability,
Understandability, and Maintainability too!

Eric Farman (10):
  s390/cio: Fix cleanup of pfn_array alloc failure
  s390/cio: Fix cleanup when unsupported IDA format is used
  s390/cio: Squash cp_free and cp_unpin_free
  s390/cio: Breakout the processing of a channel program
  s390/cio: Use common channel program processor for TIC
  s390/cio: Combine ccwchain_fetch _idal and _direct
  s390/cio: Tell pfn_array_alloc_pin to pin pages, not bytes
  s390/cio: Split pfn_array_alloc_pin into pieces
  s390/cio: Eliminate the pfn_array_table struct
  s390/cio: Remove unused function/variables FIXUP

 drivers/s390/cio/vfio_ccw_cp.c | 416 +++++++++++++++--------------------------
 drivers/s390/cio/vfio_ccw_cp.h |   1 +
 2 files changed, 150 insertions(+), 267 deletions(-)

Comments

Cornelia Huck Nov. 9, 2018, 9:13 a.m. UTC | #1
On Fri,  9 Nov 2018 03:39:27 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> Hi Connie, Pierre,
> 
> This is a first round of rework to vfio-ccw that I've been cooking to
> make the channel program handler easier to understand.  My apologies
> for the number of patches; as there are some intersections within them
> I couldn't break them up without forcing them to be applied in a
> particular order.  I imagine that since there will probably need to be
> some discussion, an RFC tag makes sense and one giant series will be
> fine for now.  I can break them up for later versions if desired.

Ten patches isn't really excessive, I think :)

> 
> (Sidebar: I'm leaving next Thursday for an early start on the US
> Thanksgiving holiday.  So if you are busy and can't get to this,
> no problem!  Now where did I leave Pierre's rework patches...  :)
> 
> Patch summary:
> 
> 1-2:	Fixes for cleanup on error paths

These look like candidates for 4.20.

> 3:	Code simplication
> 4-6:	Simplify the ccwchain processing
> 7-10:	Simplify the pfn_array processing
> 
> The first few patches are very straightforward, if unlikely to occur.
> They could even be considered candidates for 4.20, if so inclined.

Yes, the first two are, I think.

> 
> The ccwchain patches are the result of two observations about how we
> currently process a channel program and its component CCWs.  One is a
> duplicate set of code for a Transfer In Channel (TIC) CCW versus any
> other type, which provides an easy opportunity for simplification when
> all it's trying to handle is a memory jump.  The other is a duplication
> between a direct-addressed CCW and an Indirect Data Address (IDA) CCW.
> There is currently the nuance that the direct CCW path creates:
> 
>   ch_pat->pfn_array_table[1]->pfn_array[#pages]
> 
> while an IDA CCW creates:
> 
>   ch_pat->pfn_array_table[#idaws]->pfn_array[1]
> 
> It might not be obvious, but #pages is calculated identically to #idaws,
> as highlighted by patch 7.  By removing these duplications, we set the
> stage for the next patches, which can eliminate an array entirely.
> 
> The pfn_array patches permit a non-contiguous array of addresses to be
> passed to pfn_array_alloc(_pin), which in turn permits the removal of
> struct pfn_array_table and a squashing of our arrays.  Thus, instead of
> either of the two nested arrays described above, we have:
> 
>   ch_pa->pfn_array[#idaws]
> 
> (Doesn't that look nice?!)

:)

> 
> The last two patches in this series should be squashed together; I just
> left them apart in the RFC to make it easier to see how turn-key this
> becomes after the complexities of the previous patches, and without the
> noise of realigning the code afterward.  (Sorry, kbuild robot.)
> 
> While this series provides no functionality changes, I find that
> sizeof(vfio_ccw.ko) decreases by about 7 percent according to the
> bloat-o-meter, and we remove some of the malloc load for a given
> channel program.  Hopefully there's also an increase in Readability,
> Understandability, and Maintainability too!

Sounds cool! The first two patches can probably be handled quickly;
I'll see when I find time to look at the others.

(And time to look at Pierre's patches. And posting mine. Sigh.)

> 
> Eric Farman (10):
>   s390/cio: Fix cleanup of pfn_array alloc failure
>   s390/cio: Fix cleanup when unsupported IDA format is used
>   s390/cio: Squash cp_free and cp_unpin_free
>   s390/cio: Breakout the processing of a channel program
>   s390/cio: Use common channel program processor for TIC
>   s390/cio: Combine ccwchain_fetch _idal and _direct
>   s390/cio: Tell pfn_array_alloc_pin to pin pages, not bytes
>   s390/cio: Split pfn_array_alloc_pin into pieces
>   s390/cio: Eliminate the pfn_array_table struct
>   s390/cio: Remove unused function/variables FIXUP
> 
>  drivers/s390/cio/vfio_ccw_cp.c | 416 +++++++++++++++--------------------------
>  drivers/s390/cio/vfio_ccw_cp.h |   1 +
>  2 files changed, 150 insertions(+), 267 deletions(-)
>
Cornelia Huck Nov. 9, 2018, 10:57 a.m. UTC | #2
On Fri, 9 Nov 2018 10:13:32 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Fri,  9 Nov 2018 03:39:27 +0100
> Eric Farman <farman@linux.ibm.com> wrote:

> > Patch summary:
> > 
> > 1-2:	Fixes for cleanup on error paths  
> 
> These look like candidates for 4.20.

Patches look good; I plan to queue them, but would not mind an ack from
Halil as well. (Or a review from anyone else, of course :)
Farhan Ali Nov. 9, 2018, 9:22 p.m. UTC | #3
On 11/09/2018 05:57 AM, Cornelia Huck wrote:
> On Fri, 9 Nov 2018 10:13:32 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
>> On Fri,  9 Nov 2018 03:39:27 +0100
>> Eric Farman <farman@linux.ibm.com> wrote:
> 
>>> Patch summary:
>>>
>>> 1-2:	Fixes for cleanup on error paths
>>
>> These look like candidates for 4.20.
> 
> Patches look good; I plan to queue them, but would not mind an ack from
> Halil as well. (Or a review from anyone else, of course :)
> 
> 
I plan on reviewing the entire series soon :)

Thanks
Farhan
Cornelia Huck Nov. 12, 2018, 3:54 p.m. UTC | #4
On Fri,  9 Nov 2018 03:39:27 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> Patch summary:
> 
> 1-2:	Fixes for cleanup on error paths
> 3:	Code simplication
> 4-6:	Simplify the ccwchain processing
> 7-10:	Simplify the pfn_array processing

I have looked over the remaining patches (not in detail, though), and
they look like a nice cleanup.

Let's wait what others have to say :) This is post-4.20 material anyway.
Pierre Morel Nov. 14, 2018, 6:30 p.m. UTC | #5
On 09/11/2018 03:39, Eric Farman wrote:
> Hi Connie, Pierre,
> 
> This is a first round of rework to vfio-ccw that I've been cooking to
> make the channel program handler easier to understand.  My apologies
> for the number of patches; as there are some intersections within them
> I couldn't break them up without forcing them to be applied in a
> particular order.  I imagine that since there will probably need to be
> some discussion, an RFC tag makes sense and one giant series will be
> fine for now.  I can break them up for later versions if desired.
> 
> (Sidebar: I'm leaving next Thursday for an early start on the US
> Thanksgiving holiday.  So if you are busy and can't get to this,
> no problem!  Now where did I leave Pierre's rework patches...  :)
> 
> Patch summary:
> 
> 1-2:	Fixes for cleanup on error paths
> 3:	Code simplication

I am absolutely OK with these three first patches.


> 4-6:	Simplify the ccwchain processing
> 7-10:	Simplify the pfn_array processing

I still need some investigation on the last 7 ones.
On a first view, it looks generally good but I developed some ideas on 
the subject you address that you may want to study and may be implement 
as part of this memory optimization series:

- avoiding the multiples CCW chain alloc/free (already discussed in the 
dedicated patch)

- optimize the allocations of CCW chain metadata using kmem_caches
or
- dedicate a pre-allocated page to handle CCW chains in a single copy 
per subchain.
we would certainly never need more than a single page.



Regards,
Pierre