| Message ID | 20181109023937.96105-1-farman@linux.ibm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | vfio-ccw channel program rework | expand |
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(-) >
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 :)
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
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.
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