Re: [PATCH RFC 1/2] scatterlist: add mempool based chained SG alloc/free api
On Thu, Apr 7, 2016 at 9:43 AM, Ming Linwrote: > On Thu, Apr 7, 2016 at 7:56 AM, Bart Van Assche > wrote: >> On 03/15/16 15:39, Ming Lin wrote: >>> >>> +static void sg_mempoll_free(struct scatterlist *sgl, unsigned int nents) >> >> >> Please change mempoll into mempool. > > Good catch. Thanks Bart! Hi Bart, This is actually the previous old RFC patch. The v2 and v3 patch series doesn't have this typo. Thanks.
Re: [PATCH RFC 1/2] scatterlist: add mempool based chained SG alloc/free api
On Thu, Apr 7, 2016 at 9:43 AM, Ming Lin wrote: > On Thu, Apr 7, 2016 at 7:56 AM, Bart Van Assche > wrote: >> On 03/15/16 15:39, Ming Lin wrote: >>> >>> +static void sg_mempoll_free(struct scatterlist *sgl, unsigned int nents) >> >> >> Please change mempoll into mempool. > > Good catch. Thanks Bart! Hi Bart, This is actually the previous old RFC patch. The v2 and v3 patch series doesn't have this typo. Thanks.
Re: [PATCH RFC 1/2] scatterlist: add mempool based chained SG alloc/free api
On Thu, Apr 7, 2016 at 7:56 AM, Bart Van Asschewrote: > On 03/15/16 15:39, Ming Lin wrote: >> >> +static void sg_mempoll_free(struct scatterlist *sgl, unsigned int nents) > > > Please change mempoll into mempool. Good catch. Thanks Bart!
Re: [PATCH RFC 1/2] scatterlist: add mempool based chained SG alloc/free api
On Thu, Apr 7, 2016 at 7:56 AM, Bart Van Assche wrote: > On 03/15/16 15:39, Ming Lin wrote: >> >> +static void sg_mempoll_free(struct scatterlist *sgl, unsigned int nents) > > > Please change mempoll into mempool. Good catch. Thanks Bart!
Re: [PATCH RFC 1/2] scatterlist: add mempool based chained SG alloc/free api
On 03/15/16 15:39, Ming Lin wrote: +static void sg_mempoll_free(struct scatterlist *sgl, unsigned int nents) Please change mempoll into mempool. Thanks, Bart.
Re: [PATCH RFC 1/2] scatterlist: add mempool based chained SG alloc/free api
On 03/15/16 15:39, Ming Lin wrote: +static void sg_mempoll_free(struct scatterlist *sgl, unsigned int nents) Please change mempoll into mempool. Thanks, Bart.
Re: [PATCH RFC 1/2] scatterlist: add mempool based chained SG alloc/free api
On Sun, Mar 20, 2016 at 11:55:17PM -0700, Ming Lin wrote: > On Wed, 2016-03-16 at 09:23 +0100, Christoph Hellwig wrote: > > > > > We can defintively kill this one. > > We want to support different size of pools. > How can we kill this one? > > Or did you mean we just create a single pool with size SG_CHUNK_SIZE? I mean just killing the SG_MEMPOOL_NR define and using the ARRAY_SIZE directly. > I created lib/sg_pool.c with CONFIG_SG_POOL. Great!
Re: [PATCH RFC 1/2] scatterlist: add mempool based chained SG alloc/free api
On Sun, Mar 20, 2016 at 11:55:17PM -0700, Ming Lin wrote: > On Wed, 2016-03-16 at 09:23 +0100, Christoph Hellwig wrote: > > > > > We can defintively kill this one. > > We want to support different size of pools. > How can we kill this one? > > Or did you mean we just create a single pool with size SG_CHUNK_SIZE? I mean just killing the SG_MEMPOOL_NR define and using the ARRAY_SIZE directly. > I created lib/sg_pool.c with CONFIG_SG_POOL. Great!
Re: [PATCH RFC 1/2] scatterlist: add mempool based chained SG alloc/free api
On Wed, 2016-03-16 at 09:23 +0100, Christoph Hellwig wrote: > > > We can defintively kill this one. We want to support different size of pools. How can we kill this one? Or did you mean we just create a single pool with size SG_CHUNK_SIZE? > > > +static __init int sg_mempool_init(void) > > +{ > > + int i; > > + > > + for (i = 0; i < SG_MEMPOOL_NR; i++) { > > + struct sg_mempool *sgp = sg_pools + i; > > + int size = sgp->size * sizeof(struct scatterlist); > > + > > + sgp->slab = kmem_cache_create(sgp->name, size, 0, > > + SLAB_HWCACHE_ALIGN, NULL); > > Having these mempoools around in every kernel will make some embedded > developers rather unhappy. We could either not create them at > runtime, which would require either a check in the fast path, or > an init call in every driver, or just move the functions you > added into a separe file, which will be compiled only based on a > Kconfig > symbol, and could even be potentially modular. I think that > second option might be easier. I created lib/sg_pool.c with CONFIG_SG_POOL.
Re: [PATCH RFC 1/2] scatterlist: add mempool based chained SG alloc/free api
On Wed, 2016-03-16 at 09:23 +0100, Christoph Hellwig wrote: > > > We can defintively kill this one. We want to support different size of pools. How can we kill this one? Or did you mean we just create a single pool with size SG_CHUNK_SIZE? > > > +static __init int sg_mempool_init(void) > > +{ > > + int i; > > + > > + for (i = 0; i < SG_MEMPOOL_NR; i++) { > > + struct sg_mempool *sgp = sg_pools + i; > > + int size = sgp->size * sizeof(struct scatterlist); > > + > > + sgp->slab = kmem_cache_create(sgp->name, size, 0, > > + SLAB_HWCACHE_ALIGN, NULL); > > Having these mempoools around in every kernel will make some embedded > developers rather unhappy. We could either not create them at > runtime, which would require either a check in the fast path, or > an init call in every driver, or just move the functions you > added into a separe file, which will be compiled only based on a > Kconfig > symbol, and could even be potentially modular. I think that > second option might be easier. I created lib/sg_pool.c with CONFIG_SG_POOL.
Re: [PATCH RFC 1/2] scatterlist: add mempool based chained SG alloc/free api
> /* > + * The maximum number of SG segments that we will put inside a > + * scatterlist. > + * > + * XXX: what's the best number? > + */ > +#define SG_MAX_SEGMENTS 128 The important part here is that it's the amount we fit into a single scatterlist chunk. So I think naming it SG_CHUNK_SIZE or similar would be a better idea (the name in SCSI is from the days before we supported chained S/G lists). It would also be good to ⅺring over the comments from SCSI_MAX_SG_SEGMENTS. We'll also need a symbol like SCSI_MAX_SG_CHAIN_SEGMENTS that contains the absolute limit, and we need the CONFIG_ARCH_HAS_SG_CHAIN magic around it for now as we still have architetures that do not support S/G chanining in their dma_map_sg implementation. I plan to fix that up in a merge window or two, though. My name suggestion for that would be SG_MAX_SEGMENTS. > +#define SG_MEMPOOL_NRARRAY_SIZE(sg_pools) We can defintively kill this one. > +#define SG_MEMPOOL_SIZE 2 > + > +struct sg_mempool { I'd keep this as struct sg_pool, similar to SCSI. > +/** > + * sg_free_chained - Free a previously mapped sg table > + * @table: The sg table header to use > + * @first_chunk: was first_chunk not NULL in sg_alloc_chained? > + * > + * Description: > + *Free an sg table previously allocated and setup with > + *sg_alloc_chained(). > + * > + **/ > +void sg_free_chained(struct sg_table *table, bool first_chunk) Can we call this sg_free_table_chained to be similar to sg_table_free? Same for the alloc side. > +static __init int sg_mempool_init(void) > +{ > + int i; > + > + for (i = 0; i < SG_MEMPOOL_NR; i++) { > + struct sg_mempool *sgp = sg_pools + i; > + int size = sgp->size * sizeof(struct scatterlist); > + > + sgp->slab = kmem_cache_create(sgp->name, size, 0, > + SLAB_HWCACHE_ALIGN, NULL); Having these mempoools around in every kernel will make some embedded developers rather unhappy. We could either not create them at runtime, which would require either a check in the fast path, or an init call in every driver, or just move the functions you added into a separe file, which will be compiled only based on a Kconfig symbol, and could even be potentially modular. I think that second option might be easier.
Re: [PATCH RFC 1/2] scatterlist: add mempool based chained SG alloc/free api
> /* > + * The maximum number of SG segments that we will put inside a > + * scatterlist. > + * > + * XXX: what's the best number? > + */ > +#define SG_MAX_SEGMENTS 128 The important part here is that it's the amount we fit into a single scatterlist chunk. So I think naming it SG_CHUNK_SIZE or similar would be a better idea (the name in SCSI is from the days before we supported chained S/G lists). It would also be good to ⅺring over the comments from SCSI_MAX_SG_SEGMENTS. We'll also need a symbol like SCSI_MAX_SG_CHAIN_SEGMENTS that contains the absolute limit, and we need the CONFIG_ARCH_HAS_SG_CHAIN magic around it for now as we still have architetures that do not support S/G chanining in their dma_map_sg implementation. I plan to fix that up in a merge window or two, though. My name suggestion for that would be SG_MAX_SEGMENTS. > +#define SG_MEMPOOL_NRARRAY_SIZE(sg_pools) We can defintively kill this one. > +#define SG_MEMPOOL_SIZE 2 > + > +struct sg_mempool { I'd keep this as struct sg_pool, similar to SCSI. > +/** > + * sg_free_chained - Free a previously mapped sg table > + * @table: The sg table header to use > + * @first_chunk: was first_chunk not NULL in sg_alloc_chained? > + * > + * Description: > + *Free an sg table previously allocated and setup with > + *sg_alloc_chained(). > + * > + **/ > +void sg_free_chained(struct sg_table *table, bool first_chunk) Can we call this sg_free_table_chained to be similar to sg_table_free? Same for the alloc side. > +static __init int sg_mempool_init(void) > +{ > + int i; > + > + for (i = 0; i < SG_MEMPOOL_NR; i++) { > + struct sg_mempool *sgp = sg_pools + i; > + int size = sgp->size * sizeof(struct scatterlist); > + > + sgp->slab = kmem_cache_create(sgp->name, size, 0, > + SLAB_HWCACHE_ALIGN, NULL); Having these mempoools around in every kernel will make some embedded developers rather unhappy. We could either not create them at runtime, which would require either a check in the fast path, or an init call in every driver, or just move the functions you added into a separe file, which will be compiled only based on a Kconfig symbol, and could even be potentially modular. I think that second option might be easier.