Re: [PATCH v4 4/8] s390/airq: use DMA memory for adapter interrupts

[Halil Pasic <pasic@linux.ibm.com>]

On Tue, 11 Jun 2019 18:19:44 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 11 Jun 2019 16:27:21 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Tue, 11 Jun 2019 12:17:21 +0200
> > Cornelia Huck <cohuck@redhat.com> wrote:
> > 
> > > On Thu,  6 Jun 2019 13:51:23 +0200
> > > Halil Pasic <pasic@linux.ibm.com> wrote:
> > >   
> > > > Protected virtualization guests have to use shared pages for airq
> > > > notifier bit vectors, because hypervisor needs to write these bits.
> > > > 
> > > > Let us make sure we allocate DMA memory for the notifier bit vectors by
> > > > replacing the kmem_cache with a dma_cache and kalloc() with
> > > > cio_dma_zalloc().
> > > > 
> > > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > > > Reviewed-by: Sebastian Ott <sebott@linux.ibm.com>
> > > > ---
> > > >  arch/s390/include/asm/airq.h |  2 ++
> > > >  drivers/s390/cio/airq.c      | 32 ++++++++++++++++++++------------
> > > >  drivers/s390/cio/cio.h       |  2 ++
> > > >  drivers/s390/cio/css.c       |  1 +
> > > >  4 files changed, 25 insertions(+), 12 deletions(-)
> > > >   
> > > 
> > > (...)
> > >   
> > > > @@ -295,12 +303,12 @@ unsigned long airq_iv_scan(struct airq_iv *iv, unsigned long start,
> > > >  }
> > > >  EXPORT_SYMBOL(airq_iv_scan);
> > > >  
> > > > -static int __init airq_init(void)
> > > > +int __init airq_init(void)
> > > >  {
> > > > -	airq_iv_cache = ) "airq_iv_cache", cache_line_size(),
> > > > -					  cache_line_size(), 0, NULL);
> > > > +	airq_iv_cache = dma_pool_create("airq_iv_cache", cio_get_dma_css_dev(),
> > > > +					cache_line_size(),
> > > > +					cache_line_size(), PAGE_SIZE);
> > > >  	if (!airq_iv_cache)
> > > >  		return -ENOMEM;  
> > > 
> > > Sorry about not noticing that in the last iteration; but you may return
> > > an error here if airq_iv_cache could not be allocated...
> > >   
> > > >  	return 0;
> > > >  }
> > > > -subsys_initcall(airq_init);  
> > > 
> > > (...)
> > >   
> > > > diff --git a/drivers/s390/cio/css.c b/drivers/s390/cio/css.c
> > > > index 6fc91d534af1..7901c8ed3597 100644
> > > > --- a/drivers/s390/cio/css.c
> > > > +++ b/drivers/s390/cio/css.c
> > > > @@ -1182,6 +1182,7 @@ static int __init css_bus_init(void)
> > > >  	ret = cio_dma_pool_init();
> > > >  	if (ret)
> > > >  		goto out_unregister_pmn;
> > > > +	airq_init();  
> > > 
> > > ...but don't check the return code here. Probably a pathological case,
> > > but shouldn't you handle that error as well?
> > >   
> > 
> > Tricky business... The problem is that the airq stuff ain't 'private' to
> > the CIO subsystem (e.g. zPCI). I'm afraid failing to init css won't
> > really prevent all usages.
> 
> Architecture-wise, there's an unfortunate tie-in of some things like
> zPCI with the channel subsystem (most of that seems to come in via chsc
> and machine checks; but as you say, airq as well). I'd basically
> consider css to be a base system for virtually any I/O on s390...
> 

I tend to agree.

> > 
> > My first thought was, that this is more or less analogous to what we
> > had before. Namely kmem_cache_create() and dma_pool_create() should fail
> > under similar circumstances, and the return value of airq_init() was
> > ignored in do_initcall_level(). So I was like ignoring it seems to be
> > consistent with previous state.
> > 
> > But, ouch, there is a big difference! While kmem_cache_zalloc() seems
> > to tolerate the first argument (pointer to kmem_cache) being NULL the
> > dma_pool_zalloc() does not.
> 
> Yeah. While previously continuing with a failed allocation simply was
> not very workable, now we actually would end up with crashes :(
> 

Nod.

> > 
> > IMHO the cleanest thing to do at this stage is to check if the
> > airq_iv_cache is NULL and fail the allocation if it is (to preserve
> > previous behavior).
> 
> That's probably the least invasive fix for now. Did you check whether
> any of the other dma pools this series introduces have a similar
> problem due to init not failing?
>

Good question!

I did a quick check. virtio_ccw_init() should be OK, because we don't
register the driver if allocation fails, so the thing is going to end
up dysfunctional as expected.

If however cio_dma_pool_init() fails, then we end up with the same
problem with airqs, just on the !AIRQ_IV_CACHELINE code path. It can be
fixed analogously: make cio_dma_zalloc() fail all allocation if
cio_dma_pool_init() failed before.

The rest should be OK.

> > 
> > I would prefer having a separate discussion on eventually changing
> > the behavior (e.g. fail css initialization).
> 
> I did a quick check of the common I/O layer code and one place that
> looks dangerous is the chsc initialization (where we get two pages that
> are later accessed unconditionally by the code).
> 
> All of this is related to not being able to fulfill some basic memory
> availability requirements early during boot and then discovering that
> pulling the emergency break did not actually stop the train. I'd vote
> for calling panic() if the common I/O layer cannot perform its setup;
> but as this is really a pathological case I also think we should solve
> that independently of this patch series.
>

panic() sounds very reasonable to me. As an user I would like to see a
message that tells me, I'm trying to boot with insufficient RAM. Is there
such a message somewhere?
 
> > 
> > Connie, would that work with you? Thanks for spotting this!
> 
> Yeah, let's give your approach a try.
> 

OK. I intend to send out v5 with these changes tomorrow in the
afternoon:
 
diff --git a/drivers/s390/cio/airq.c b/drivers/s390/cio/airq.c
index 89d26e43004d..427b2e24a8ce 100644
--- a/drivers/s390/cio/airq.c
+++ b/drivers/s390/cio/airq.c
@@ -142,7 +142,8 @@ struct airq_iv *airq_iv_create(unsigned long bits, unsigned long flags)
        size = iv_size(bits);
 
        if (flags & AIRQ_IV_CACHELINE) {
-               if ((cache_line_size() * BITS_PER_BYTE) < bits)
+               if ((cache_line_size() * BITS_PER_BYTE) < bits
+                               || !airq_iv_cache)
                        goto out_free;
 
                iv->vector = dma_pool_zalloc(airq_iv_cache, GFP_KERNEL,
@@ -186,7 +187,7 @@ struct airq_iv *airq_iv_create(unsigned long bits, unsigned long flags)
        kfree(iv->ptr);
        kfree(iv->bitlock);
        kfree(iv->avail);
-       if (iv->flags & AIRQ_IV_CACHELINE)
+       if (iv->flags & AIRQ_IV_CACHELINE && iv->vector)
                dma_pool_free(airq_iv_cache, iv->vector, iv->vector_dma);
        else
                cio_dma_free(iv->vector, size);
diff --git a/drivers/s390/cio/css.c b/drivers/s390/cio/css.c
index 7901c8ed3597..d709bd8545f2 100644
--- a/drivers/s390/cio/css.c
+++ b/drivers/s390/cio/css.c
@@ -1128,6 +1128,8 @@ void cio_gp_dma_free(struct gen_pool *gp_dma, void *cpu_addr, size_t size)
  */
 void *cio_dma_zalloc(size_t size)
 {
+       if (!cio_dma_pool)
+               return NULL;
        return cio_gp_dma_zalloc(cio_dma_pool, cio_get_dma_css_dev(), size);
 }