Re: [PATCH 2/2] scsi: core: avoid to pre-allocate big chunk for sg list

[Bart Van Assche <bvanassche@acm.org>]

On Wed, 2019-04-24 at 08:24 -0700, James Bottomley wrote:
> On Wed, 2019-04-24 at 15:52 +0800, Ming Lei wrote:
> > On Tue, Apr 23, 2019 at 08:37:15AM -0700, Bart Van Assche wrote:
> > > On Tue, 2019-04-23 at 18:32 +0800, Ming Lei wrote:
> > > >  #define  SCSI_INLINE_PROT_SG_CNT  1
> > > >  
> > > > +#define  SCSI_INLINE_SG_CNT  2
> > > 
> > > So this patch inserts one kmalloc() and one kfree() call in the hot
> > > path for every SCSI request with more than two elements in its
> > > scatterlist? Isn't
> > 
> > Slab or its variants are designed for fast path, and NVMe PCI uses
> > slab for allocating sg list in fast path too.
> 
> Actually, that's not really true  base kmalloc can do all sorts of
> things including kick off reclaim so it's not really something we like
> using in the fast path.  The only fast and safe kmalloc you can rely on
>  in the fast path is GFP_ATOMIC which will fail quickly if no memory
> can easily be found.  *However* the sg_table allocation functions are
> all pool backed (lib/sg_pool.c), so they use the lightweight GFP_ATOMIC
> mechanism for kmalloc initially coupled with a backing pool in case of
> failure to ensure forward progress.
> 
> So, I think you're both right: you shouldn't simply use kmalloc, but
> this implementation doesn't, it uses the sg_table allocation functions
> which correctly control kmalloc to be lightweight and efficient and
> able to make forward progress.

Another concern is whether this change can cause a livelock. If the system
is running out of memory and the page cache submits a write request with
a scatterlist with more than two elements, if the kmalloc() for the
scatterlist fails, will that prevent the page cache from making any progress
with writeback?

Bart.