[PATCH 0/2] scis: core: avoid big pre-allocation for sg list

[PATCH 0/2] scis: core: avoid big pre-allocation for sg list

[Ming Lei]

Hi,

Since supporting to blk-mq, big pre-allocation for sg list is
introduced, this way is very unfriendly wrt. memory consumption.

There were Red Hat internal reports that some scsi_debug based tests
can't be run because of too big pre-allocation.

Also lpfc users commplained that 1GB+ ram is pre-allocatd for single
HBA.

The two patches try to address this issue by allocating sg list runtime,
meantime pre-allocating one or two inline sg entries for small IO. This
ways follows NVMe's approach wrt. sg list allocation.


Ming Lei (2):
  scsi: core: avoid to pre-allocate big chunk for protection meta data
  scsi: core: avoid to pre-allocate big chunk for sg list

 drivers/scsi/scsi_lib.c | 60 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 46 insertions(+), 14 deletions(-)

Cc: Christoph Hellwig <hch@lst.de>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Ewan D. Milne <emilne@redhat.com>
Cc: Hannes Reinecke <hare@suse.com>

-- 
2.9.5

[PATCH 1/2] scsi: core: avoid to pre-allocate big chunk for protection meta data

[Ming Lei]

Now scsi_mq_setup_tags() pre-allocates a big buffer for protection
sg list, and the buffer size is scsi_mq_sgl_size().

This way isn't correct, scsi_mq_sgl_size() is used to pre-allocate
sg list for IO data. And the protection data buffer is much less,
for example, one 512byte sector needs 8byte protection data, and
the max sector number for one request is 2560(BLK_DEF_MAX_SECTORS),
so the max protection data size is just 20k.

The usual case is that one bio builds one single bip segment. Attribute
to bio split, bio merge is seldom done for big IO, and it is only done
in case of small bios. And the bip segment number is usually same with
bio count in the request, so the number won't be very big, and
allocating from slab should be fast enough.

Reduce to pre-alocate one sg entry for protection data, and switches
to runtime allocation from slab in case that the protection data
segment number is bigger than 1. So we can save huge pre-alocation
for protection data, for example, 500+MB can be saved on lpfc.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Ewan D. Milne <emilne@redhat.com>
Cc: Hannes Reinecke <hare@suse.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/scsi_lib.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 07dfc17d4824..bdcf40851356 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -39,6 +39,12 @@
 #include "scsi_priv.h"
 #include "scsi_logging.h"
 
+/*
+ * Size of integrity meta data size is usually small, 1 inline sg
+ * should cover normal cases.
+ */
+#define  SCSI_INLINE_PROT_SG_CNT  1
+
 static struct kmem_cache *scsi_sdb_cache;
 static struct kmem_cache *scsi_sense_cache;
 static struct kmem_cache *scsi_sense_isadma_cache;
@@ -553,12 +559,21 @@ static void scsi_uninit_cmd(struct scsi_cmnd *cmd)
 	}
 }
 
+static inline bool scsi_prot_use_inline_sg(struct scsi_cmnd *cmd)
+{
+	if (!scsi_prot_sglist(cmd))
+		return false;
+
+	return cmd->prot_sdb->table.sgl ==
+		(struct scatterlist *)(cmd->prot_sdb + 1);
+}
+
 static void scsi_mq_free_sgtables(struct scsi_cmnd *cmd)
 {
 	if (cmd->sdb.table.nents)
 		sg_free_table_chained(&cmd->sdb.table, true);
-	if (scsi_prot_sg_count(cmd))
-		sg_free_table_chained(&cmd->prot_sdb->table, true);
+	if (scsi_prot_sg_count(cmd) && !scsi_prot_use_inline_sg(cmd))
+		sg_free_table_chained(&cmd->prot_sdb->table, false);
 }
 
 static void scsi_mq_uninit_cmd(struct scsi_cmnd *cmd)
@@ -1044,9 +1059,11 @@ blk_status_t scsi_init_io(struct scsi_cmnd *cmd)
 		}
 
 		ivecs = blk_rq_count_integrity_sg(rq->q, rq->bio);
-
-		if (sg_alloc_table_chained(&prot_sdb->table, ivecs,
-				prot_sdb->table.sgl)) {
+		if (ivecs <= SCSI_INLINE_PROT_SG_CNT)
+			prot_sdb->table.nents = prot_sdb->table.orig_nents =
+				SCSI_INLINE_PROT_SG_CNT;
+		else if (sg_alloc_table_chained(&prot_sdb->table, ivecs,
+					NULL)) {
 			ret = BLK_STS_RESOURCE;
 			goto out_free_sgtables;
 		}
@@ -1846,7 +1863,8 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
 	sgl_size = scsi_mq_sgl_size(shost);
 	cmd_size = sizeof(struct scsi_cmnd) + shost->hostt->cmd_size + sgl_size;
 	if (scsi_host_get_prot(shost))
-		cmd_size += sizeof(struct scsi_data_buffer) + sgl_size;
+		cmd_size += sizeof(struct scsi_data_buffer) +
+			sizeof(struct scatterlist) * SCSI_INLINE_PROT_SG_CNT;
 
 	memset(&shost->tag_set, 0, sizeof(shost->tag_set));
 	shost->tag_set.ops = &scsi_mq_ops;
-- 
2.9.5

Re: [PATCH 1/2] scsi: core: avoid to pre-allocate big chunk for protection meta data

[Bart Van Assche]

On Tue, 2019-04-23 at 18:32 +0800, Ming Lei wrote:
> +/*
> + * Size of integrity meta data size is usually small, 1 inline sg
                        ^^^^^^^^^^^^^^
Please change this into "metadata". Mentioning "size" twice is not
useful.
> + * should cover normal cases.
> + */
> +#define  SCSI_INLINE_PROT_SG_CNT  1
> +
>  static struct kmem_cache *scsi_sdb_cache;
>  static struct kmem_cache *scsi_sense_cache;
>  static struct kmem_cache *scsi_sense_isadma_cache;
> @@ -553,12 +559,21 @@ static void scsi_uninit_cmd(struct scsi_cmnd *cmd)
>         }
>  }
>  
> +static inline bool scsi_prot_use_inline_sg(struct scsi_cmnd *cmd)
> +{
> +       if (!scsi_prot_sglist(cmd))
> +               return false;

Since scsi_prot_use_inline_sg() is only called if scsi_prot_sg_count() > 0,
is the above test necessary?

> +       return cmd->prot_sdb->table.sgl ==
> +               (struct scatterlist *)(cmd->prot_sdb + 1);
> +}
> +
>  static void scsi_mq_free_sgtables(struct scsi_cmnd *cmd)
>  {
>         if (cmd->sdb.table.nents)
>                 sg_free_table_chained(&cmd->sdb.table, true);
> -       if (scsi_prot_sg_count(cmd))
> -               sg_free_table_chained(&cmd->prot_sdb->table, true);
> +       if (scsi_prot_sg_count(cmd) && !scsi_prot_use_inline_sg(cmd))
> +               sg_free_table_chained(&cmd->prot_sdb->table, false);
>  }
>  
>  static void scsi_mq_uninit_cmd(struct scsi_cmnd *cmd)
> @@ -1044,9 +1059,11 @@ blk_status_t scsi_init_io(struct scsi_cmnd *cmd)
>                 }
>  
>                 ivecs = blk_rq_count_integrity_sg(rq->q, rq->bio);
> -
> -               if (sg_alloc_table_chained(&prot_sdb->table, ivecs,
> -                               prot_sdb->table.sgl)) {
> +               if (ivecs <= SCSI_INLINE_PROT_SG_CNT)
> +                       prot_sdb->table.nents = prot_sdb->table.orig_nents =
> +                               SCSI_INLINE_PROT_SG_CNT;

Please call sg_init_table() instead of open-coding it.

> +               else if (sg_alloc_table_chained(&prot_sdb->table, ivecs,
> +                                       NULL)) {
>                         ret = BLK_STS_RESOURCE;
>                         goto out_free_sgtables;
>                 }

Bart.

Re: [PATCH 1/2] scsi: core: avoid to pre-allocate big chunk for protection meta data

[Ming Lei]

On Tue, Apr 23, 2019 at 08:33:47AM -0700, Bart Van Assche wrote:
> On Tue, 2019-04-23 at 18:32 +0800, Ming Lei wrote:
> > +/*
> > + * Size of integrity meta data size is usually small, 1 inline sg
>                         ^^^^^^^^^^^^^^
> Please change this into "metadata". Mentioning "size" twice is not
> useful.

OK

> > + * should cover normal cases.
> > + */
> > +#define  SCSI_INLINE_PROT_SG_CNT  1
> > +
> >  static struct kmem_cache *scsi_sdb_cache;
> >  static struct kmem_cache *scsi_sense_cache;
> >  static struct kmem_cache *scsi_sense_isadma_cache;
> > @@ -553,12 +559,21 @@ static void scsi_uninit_cmd(struct scsi_cmnd *cmd)
> >         }
> >  }
> >  
> > +static inline bool scsi_prot_use_inline_sg(struct scsi_cmnd *cmd)
> > +{
> > +       if (!scsi_prot_sglist(cmd))
> > +               return false;
> 
> Since scsi_prot_use_inline_sg() is only called if scsi_prot_sg_count() > 0,
> is the above test necessary?

Yeah, the check can be removed.

> 
> > +       return cmd->prot_sdb->table.sgl ==
> > +               (struct scatterlist *)(cmd->prot_sdb + 1);
> > +}
> > +
> >  static void scsi_mq_free_sgtables(struct scsi_cmnd *cmd)
> >  {
> >         if (cmd->sdb.table.nents)
> >                 sg_free_table_chained(&cmd->sdb.table, true);
> > -       if (scsi_prot_sg_count(cmd))
> > -               sg_free_table_chained(&cmd->prot_sdb->table, true);
> > +       if (scsi_prot_sg_count(cmd) && !scsi_prot_use_inline_sg(cmd))
> > +               sg_free_table_chained(&cmd->prot_sdb->table, false);
> >  }
> >  
> >  static void scsi_mq_uninit_cmd(struct scsi_cmnd *cmd)
> > @@ -1044,9 +1059,11 @@ blk_status_t scsi_init_io(struct scsi_cmnd *cmd)
> >                 }
> >  
> >                 ivecs = blk_rq_count_integrity_sg(rq->q, rq->bio);
> > -
> > -               if (sg_alloc_table_chained(&prot_sdb->table, ivecs,
> > -                               prot_sdb->table.sgl)) {
> > +               if (ivecs <= SCSI_INLINE_PROT_SG_CNT)
> > +                       prot_sdb->table.nents = prot_sdb->table.orig_nents =
> > +                               SCSI_INLINE_PROT_SG_CNT;
> 
> Please call sg_init_table() instead of open-coding it.

OK.

Thanks,
Ming

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

[Ming Lei]

Now scsi_mq_setup_tags() pre-allocates a big buffer for IO sg list,
and the buffer size is scsi_mq_sgl_size() which depends on smaller
value between shost->sg_tablesize and SG_CHUNK_SIZE.

Modern HBA's DMA capabilty is often capable of deadling with very
big segment number, so scsi_mq_sgl_size() is often big. Suppose the
max sg number of SG_CHUNK_SIZE is taken, scsi_mq_sgl_size() will be
4KB.

Then if one HBA has lots of queues, and each hw queue's depth is
big, the whole pre-allocation for sg list can consume huge memory.
For example of lpfc, nr_hw_queues can be 70, each queue's depth
can be 3781, so the pre-allocation for data sg list can be 70*3781*2k
=517MB for single HBA.

Also there is Red Hat internal reprot that scsi_debug based tests can't
be run any more since legacy io path is killed because too big
pre-allocation.

This patch switchs to runtime allocation for sg list, meantime
pre-allocate 2 inline sg entries. This way has been applied to
NVMe for a while, so it should be fine for SCSI too.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Ewan D. Milne <emilne@redhat.com>
Cc: Hannes Reinecke <hare@suse.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/scsi_lib.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index bdcf40851356..4fff95b14c91 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -45,6 +45,8 @@
  */
 #define  SCSI_INLINE_PROT_SG_CNT  1
 
+#define  SCSI_INLINE_SG_CNT  2
+
 static struct kmem_cache *scsi_sdb_cache;
 static struct kmem_cache *scsi_sense_cache;
 static struct kmem_cache *scsi_sense_isadma_cache;
@@ -568,10 +570,18 @@ static inline bool scsi_prot_use_inline_sg(struct scsi_cmnd *cmd)
 		(struct scatterlist *)(cmd->prot_sdb + 1);
 }
 
+static bool scsi_use_inline_sg(struct scsi_cmnd *cmd)
+{
+	struct scatterlist *sg = (void *)cmd + sizeof(struct scsi_cmnd) +
+		cmd->device->host->hostt->cmd_size;
+
+	return cmd->sdb.table.sgl == sg;
+}
+
 static void scsi_mq_free_sgtables(struct scsi_cmnd *cmd)
 {
-	if (cmd->sdb.table.nents)
-		sg_free_table_chained(&cmd->sdb.table, true);
+	if (cmd->sdb.table.nents && !scsi_use_inline_sg(cmd))
+		sg_free_table_chained(&cmd->sdb.table, false);
 	if (scsi_prot_sg_count(cmd) && !scsi_prot_use_inline_sg(cmd))
 		sg_free_table_chained(&cmd->prot_sdb->table, false);
 }
@@ -1002,12 +1012,16 @@ static blk_status_t scsi_init_sgtable(struct request *req,
 		struct scsi_data_buffer *sdb)
 {
 	int count;
+	unsigned nr_segs = blk_rq_nr_phys_segments(req);
 
 	/*
 	 * If sg table allocation fails, requeue request later.
 	 */
-	if (unlikely(sg_alloc_table_chained(&sdb->table,
-			blk_rq_nr_phys_segments(req), sdb->table.sgl)))
+	if (nr_segs <= SCSI_INLINE_SG_CNT)
+		sdb->table.nents = sdb->table.orig_nents =
+			SCSI_INLINE_SG_CNT;
+	else if (unlikely(sg_alloc_table_chained(&sdb->table, nr_segs,
+					NULL)))
 		return BLK_STS_RESOURCE;
 
 	/* 
@@ -1574,9 +1588,9 @@ static int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
 }
 
 /* Size in bytes of the sg-list stored in the scsi-mq command-private data. */
-static unsigned int scsi_mq_sgl_size(struct Scsi_Host *shost)
+static unsigned int scsi_mq_inline_sgl_size(struct Scsi_Host *shost)
 {
-	return min_t(unsigned int, shost->sg_tablesize, SG_CHUNK_SIZE) *
+	return min_t(unsigned int, shost->sg_tablesize, SCSI_INLINE_SG_CNT) *
 		sizeof(struct scatterlist);
 }
 
@@ -1766,7 +1780,7 @@ static int scsi_mq_init_request(struct blk_mq_tag_set *set, struct request *rq,
 	if (scsi_host_get_prot(shost)) {
 		sg = (void *)cmd + sizeof(struct scsi_cmnd) +
 			shost->hostt->cmd_size;
-		cmd->prot_sdb = (void *)sg + scsi_mq_sgl_size(shost);
+		cmd->prot_sdb = (void *)sg + scsi_mq_inline_sgl_size(shost);
 	}
 
 	return 0;
@@ -1860,7 +1874,7 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
 {
 	unsigned int cmd_size, sgl_size;
 
-	sgl_size = scsi_mq_sgl_size(shost);
+	sgl_size = scsi_mq_inline_sgl_size(shost);
 	cmd_size = sizeof(struct scsi_cmnd) + shost->hostt->cmd_size + sgl_size;
 	if (scsi_host_get_prot(shost))
 		cmd_size += sizeof(struct scsi_data_buffer) +
-- 
2.9.5

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

[Bart Van Assche]

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
"2" too low? Or in other words, what is the performance impact of this patch
for a real-world workload?

Thanks,

Bart.

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

[Ming Lei]

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.

> "2" too low? Or in other words, what is the performance impact of this patch
> for a real-world workload?

2 is used by NVMe PCI for a while, and even recently it is reduced to 1.

I have run big BS(256k) fio test on scsi_debug(can_queue: 256, submit queues: 12 LUNs: 4,
delay: 0, io sched: none), not see obvious performance difference by this patch.

Thanks, 
Ming

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

[James Bottomley]

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.

James

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

[Bart Van Assche]

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.

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

[Jens Axboe]

On 4/24/19 9:32 AM, Bart Van Assche wrote:
> 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?

The mempool prevents that - as long as we have at least one reserved unit
of the given size, we know we'll always make progress (eventually). The
finite life times of the sg pool allocations guarantee that.

-- 
Jens Axboe

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

[James Bottomley]

On Wed, 2019-04-24 at 08:32 -0700, Bart Van Assche wrote:
> 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?

It's pool backed, as I said.  Is the concern there isn't enough depth
in the pools for a large write?

James

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

[Bart Van Assche]

On Wed, 2019-04-24 at 08:49 -0700, James Bottomley wrote:
> On Wed, 2019-04-24 at 08:32 -0700, Bart Van Assche wrote:
> > 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?
> 
> It's pool backed, as I said.  Is the concern there isn't enough depth
> in the pools for a large write?

That memory pool is used by multiple drivers. Most but not all
sg_alloc_table_chained() calls happen from inside .queue_rq() implementations.
One sg_alloc_table_chained() call occurs in the NFS server code. I'm not sure
whether it is guaranteed that an sg_alloc_table_chained() will succeed sooner
or later under low memory conditions. Additionally, new
sg_alloc_table_chained() could be added in drivers any time.

Bart.

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

[James Bottomley]

On Wed, 2019-04-24 at 09:09 -0700, Bart Van Assche wrote:
> On Wed, 2019-04-24 at 08:49 -0700, James Bottomley wrote:
> > On Wed, 2019-04-24 at 08:32 -0700, Bart Van Assche wrote:
> > > 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?
> > 
> > It's pool backed, as I said.  Is the concern there isn't enough
> > depth in the pools for a large write?
> 
> That memory pool is used by multiple drivers. Most but not all
> sg_alloc_table_chained() calls happen from inside .queue_rq()
> implementations. One sg_alloc_table_chained() call occurs in the NFS
> server code. I'm not sure whether it is guaranteed that an
> sg_alloc_table_chained() will succeed sooner or later under low
> memory conditions. Additionally, new sg_alloc_table_chained() could
> be added in drivers any time.

The number of users is irrelevant.  All we need is sequential forward
progress to guarantee freedom from memory allocation related live lock.
 Even if they make write progress one at a time (although the current
pool depth seems to be 2, so they make progress at least two at a
time), memory will be released by the write and reclaim will progress.

The guarantee required is ability to send or have outstanding at least
one write and also that that write will return eventually releasing
memory back to the pool for another write to proceed.

James

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

[Christoph Hellwig]

On Tue, Apr 23, 2019 at 06:32:40PM +0800, Ming Lei wrote:
> big, the whole pre-allocation for sg list can consume huge memory.
> For example of lpfc, nr_hw_queues can be 70, each queue's depth
> can be 3781, so the pre-allocation for data sg list can be 70*3781*2k
> =517MB for single HBA.

We should probably limit the number of queues to something actually
useful, independent of your patch..

> +static bool scsi_use_inline_sg(struct scsi_cmnd *cmd)
> +{
> +	struct scatterlist *sg = (void *)cmd + sizeof(struct scsi_cmnd) +
> +		cmd->device->host->hostt->cmd_size;
> +
> +	return cmd->sdb.table.sgl == sg;
> +}

It might make more sense to have a helper to calculate the inline
sg address and use that for the comparism in scsi_mq_free_sgtables
and any other place that wants the address.

> +	if (cmd->sdb.table.nents && !scsi_use_inline_sg(cmd))
> +		sg_free_table_chained(&cmd->sdb.table, false);

This removes the last use of the first_chunk paramter to
sg_free_table_chained, please remove the paramter in an additional
patch.

> +	if (nr_segs <= SCSI_INLINE_SG_CNT)
> +		sdb->table.nents = sdb->table.orig_nents =
> +			SCSI_INLINE_SG_CNT;

Don't we need a sg_init_table here?

> +	else if (unlikely(sg_alloc_table_chained(&sdb->table, nr_segs,
> +					NULL)))
>  		return BLK_STS_RESOURCE;

We should probably also be able to drop the last parameter to
sg_alloc_table_chained now.

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

[Ming Lei]

On Wed, Apr 24, 2019 at 07:53:17AM +0200, Christoph Hellwig wrote:
> On Tue, Apr 23, 2019 at 06:32:40PM +0800, Ming Lei wrote:
> > big, the whole pre-allocation for sg list can consume huge memory.
> > For example of lpfc, nr_hw_queues can be 70, each queue's depth
> > can be 3781, so the pre-allocation for data sg list can be 70*3781*2k
> > =517MB for single HBA.
> 
> We should probably limit the number of queues to something actually
> useful, independent of your patch..
> 
> > +static bool scsi_use_inline_sg(struct scsi_cmnd *cmd)
> > +{
> > +	struct scatterlist *sg = (void *)cmd + sizeof(struct scsi_cmnd) +
> > +		cmd->device->host->hostt->cmd_size;
> > +
> > +	return cmd->sdb.table.sgl == sg;
> > +}
> 
> It might make more sense to have a helper to calculate the inline
> sg address and use that for the comparism in scsi_mq_free_sgtables
> and any other place that wants the address.

Good idea, and the helper can be used in scsi_mq_prep_fn() too.

> 
> > +	if (cmd->sdb.table.nents && !scsi_use_inline_sg(cmd))
> > +		sg_free_table_chained(&cmd->sdb.table, false);
> 
> This removes the last use of the first_chunk paramter to
> sg_free_table_chained, please remove the paramter in an additional
> patch.

NVMe FC/RDMA still uses first_chunk.

> 
> > +	if (nr_segs <= SCSI_INLINE_SG_CNT)
> > +		sdb->table.nents = sdb->table.orig_nents =
> > +			SCSI_INLINE_SG_CNT;
> 
> Don't we need a sg_init_table here?

Will do it in V2.

> 
> > +	else if (unlikely(sg_alloc_table_chained(&sdb->table, nr_segs,
> > +					NULL)))
> >  		return BLK_STS_RESOURCE;
> 
> We should probably also be able to drop the last parameter to
> sg_alloc_table_chained now.

NVMe FC/RDMA still uses first_chunk.

Thanks,
Ming

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

[Christoph Hellwig]

On Wed, Apr 24, 2019 at 04:41:46PM +0800, Ming Lei wrote:
> > This removes the last use of the first_chunk paramter to
> > sg_free_table_chained, please remove the paramter in an additional
> > patch.
> 
> NVMe FC/RDMA still uses first_chunk.

Looks like they need a similar treatment then :)

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

[Ming Lei]

On Wed, Apr 24, 2019 at 04:38:33PM +0200, Christoph Hellwig wrote:
> On Wed, Apr 24, 2019 at 04:41:46PM +0800, Ming Lei wrote:
> > > This removes the last use of the first_chunk paramter to
> > > sg_free_table_chained, please remove the paramter in an additional
> > > patch.
> > 
> > NVMe FC/RDMA still uses first_chunk.
> 
> Looks like they need a similar treatment then :)

Yeah, I agree.

But the issue could be less serious for NVMe FC/RDMA because the queue
depth is often much small.

thanks,
Ming