| Message ID | 9442060edaca12f520bcbb76545ad33a52346e58.1458023698.git.baolin.wang@linaro.org (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Herbert Xu |
| Headers | show |
Baolin Wang <baolin.wang@linaro.org> writes: > +/** > + * sg_is_contiguous - Check if the scatterlists are contiguous > + * @sga: SG entry > + * @sgb: SG entry > + * > + * Description: > + * If the sga scatterlist is contiguous with the sgb scatterlist, > + * that means they can be merged together. > + * > + **/ > +static inline bool sg_is_contiguous(struct scatterlist *sga, > + struct scatterlist *sgb) > +{ > + return *(unsigned long *)sg_virt(sga) + sga->length == > + *(unsigned long *)sg_virt(sgb); As I already said, I don't like casts. But let's take some height : you're needing this function to decide to merge scatterlists. That means that you think the probability of having 2 scatterlist mergeable is not meaningless, ie. 50% or more. I suppose your scatterlists are allocated through kmalloc(). I'd like to know, through your testing, what is the success rate of sg_is_contiguous(), ie. I'd like to know how many times sg_is_contiguous() was called, and amongst these calls how many times it returned true. That will tell me how "worth" is this optimization. > + * sg_add_sg_to_table - Add one scatterlist into sg table > + * @sgt: The sg table header to use > + * @src: The sg need to be added into sg table > + * > + * Description: > + * The 'nents' member indicates how many mapped scatterlists has been added > + * in the dynamic sg table. The 'orig_nents' member indicates the size of the > + * dynamic sg table. > + * > + * Copy one mapped @src@ scatterlist into the dynamic sg table and increase > + * 'nents' member. > + * > + **/ Okay, I still believe this one is wrong, because we don't understand each other. So let's take an example : sg_table = { .sgl = { { .page_link = PAGE_48, .offset = 0, .length = 2048, .dma_address = 0x30000, .dma_length = 4096, }, { .page_link = PAGE_48 | 0x02, .offset = 2048, .length = 2048, .dma_address = 0, .dma_length = 0, }, }, .nents = 1, .orig_nents = 2, }; In this example, by sheer luck the 2 scatterlist entries were physically contiguous, and the mapping function coallesced the 2 entries into only one (dma_address, dma_length) entry. That could also happen with an IOMMU by the way. Therefore, sg_table.nents = 1. If I understand your function correctly, it will erase sg_table.sgl[1], and that looks incorrect to me. This is why I can't understand how your code can be correct, and why I say you add a new "meaning" to sg_table->nents, which is not consistent with the meaning I understand. Cheers.
Hi Robert, Sorry for the late reply. On 2 April 2016 at 23:00, Robert Jarzmik <robert.jarzmik@free.fr> wrote: > Baolin Wang <baolin.wang@linaro.org> writes: > >> +/** >> + * sg_is_contiguous - Check if the scatterlists are contiguous >> + * @sga: SG entry >> + * @sgb: SG entry >> + * >> + * Description: >> + * If the sga scatterlist is contiguous with the sgb scatterlist, >> + * that means they can be merged together. >> + * >> + **/ >> +static inline bool sg_is_contiguous(struct scatterlist *sga, >> + struct scatterlist *sgb) >> +{ >> + return *(unsigned long *)sg_virt(sga) + sga->length == >> + *(unsigned long *)sg_virt(sgb); > As I already said, I don't like casts. OK. Could you give me a good example. Thanks. > > But let's take some height : you're needing this function to decide to merge > scatterlists. That means that you think the probability of having 2 scatterlist > mergeable is not meaningless, ie. 50% or more. > > I suppose your scatterlists are allocated through kmalloc(). I'd like to know, > through your testing, what is the success rate of sg_is_contiguous(), ie. I'd > like to know how many times sg_is_contiguous() was called, and amongst these > calls how many times it returned true. > > That will tell me how "worth" is this optimization. I think this is just one useful API for users, If the rate is only 1%, we also need to check if they are contiguous to decide if they can be coalesced. > >> + * sg_add_sg_to_table - Add one scatterlist into sg table >> + * @sgt: The sg table header to use >> + * @src: The sg need to be added into sg table >> + * >> + * Description: >> + * The 'nents' member indicates how many mapped scatterlists has been added >> + * in the dynamic sg table. The 'orig_nents' member indicates the size of the >> + * dynamic sg table. >> + * >> + * Copy one mapped @src@ scatterlist into the dynamic sg table and increase >> + * 'nents' member. >> + * >> + **/ > Okay, I still believe this one is wrong, because we don't understand each other. > So let's take an example : > sg_table = { > .sgl = { > { > .page_link = PAGE_48, > .offset = 0, > .length = 2048, > .dma_address = 0x30000, > .dma_length = 4096, > }, > { > .page_link = PAGE_48 | 0x02, > .offset = 2048, > .length = 2048, > .dma_address = 0, > .dma_length = 0, > }, > }, > .nents = 1, > .orig_nents = 2, > }; > > In this example, by sheer luck the 2 scatterlist entries were physically > contiguous, and the mapping function coallesced the 2 entries into only one > (dma_address, dma_length) entry. That could also happen with an IOMMU by the > way. Therefore, sg_table.nents = 1. > > If I understand your function correctly, it will erase sg_table.sgl[1], and that > looks incorrect to me. This is why I can't understand how your code can be > correct, and why I say you add a new "meaning" to sg_table->nents, which is not > consistent with the meaning I understand. I think you misunderstood my point. The 'sg_add_sg_to_table()' function is used to add one mapped scatterlist into the dynamic sg table. For example: 1. How to add one mapped sg into dynamic sg table (1) If we allocate one dynamic sg table with 3 (small size can be showed easily) empty scatterlists.: sg_table = { .sgl = { { .page_link = 0, .offset = 0, .length = 0, }, { .page_link = 0, .offset = 0, .length = 0, }, { .page_link = 0 | 0x02, .offset = 0, .length = 0, }, }, .nents = 0, .orig_nents = 3, }; (2) If some module (like dm-crypt module) always sends one mapped scatterlist (size is always 512bytes) to another module (crypto driver) to handle the mapped scatterlist at one time. But we want to collect the mapped scatterlist into one dynamic sg table to manage them together, means we can send multiple mapped scatterlists (from sg_table->sgl) to the crypto driver to handle them together to improve its efficiency. So we add one mapped scatterlists into the dynamic sg table. mapped sg = { .page_link = PAGE_20, .offset = 0, .length = 512, }, Add into synamic sg table -------> sg_table = { .sgl = { { .page_link = PAGE_20 | 0x02, .offset = 0, .length = 512, }, { .page_link = 0, .offset = 0, .length = 0, }, { .page_link = 0, .offset = 0, .length = 0, }, }, .nents = 1, .orig_nents = 3, }; Another two mapped scatterlists are added into synamic sg table -------> sg_table = { .sgl = { { .page_link = PAGE_20, .offset = 0, .length = 512, }, { .page_link = PAGE_25, .offset = 0, .length = 512, }, { .page_link = PAGE_28 | 0x20, .offset = 0, .length = 512, }, }, .nents = 3, .orig_nents = 3, }; Then we can send the dynamic sg table to the crypto engine driver to handle them together at one time. (If the dynamic sg table size is 512, then the crypto engine driver can handle 512 scatterlists at one time, which can improve much efficiency). That's the reason why we want to introduce the dynamic sg table. 2. How to coalesce (1) If one mapped scatterlsit already has been added into dynamic sg table: sg_table = { .sgl = { { .page_link = PAGE_20 | 0x02, .offset = 0, .length = 512, }, { .page_link = 0, .offset = 0, .length = 0, }, { .page_link = 0, .offset = 0, .length = 0, }, }, .nents = 1, .orig_nents = 3, }; (2) Another mapped scatterlist comes. mapped sg = { .page_link = PAGE_20, .offset = 512, .length = 512, }, So we check the new mapped scatterlist can be coalesced into previous one in dynamic sg table like this: sg_table = { .sgl = { { .page_link = PAGE_20 | 0x02, .offset = 0, .length = 1024, }, { .page_link = 0, .offset = 0, .length = 0, }, { .page_link = 0, .offset = 0, .length = 0, }, }, .nents = 1, .orig_nents = 3, }; It's done. We coalesced one scatterlist into antoher one to expand the scatterlist's length. Thanks for your comments. > > Cheers. > > -- > Robert
Hi Robert, On 5 April 2016 at 15:10, Baolin Wang <baolin.wang@linaro.org> wrote: > Hi Robert, > > Sorry for the late reply. > > On 2 April 2016 at 23:00, Robert Jarzmik <robert.jarzmik@free.fr> wrote: >> Baolin Wang <baolin.wang@linaro.org> writes: >> >>> +/** >>> + * sg_is_contiguous - Check if the scatterlists are contiguous >>> + * @sga: SG entry >>> + * @sgb: SG entry >>> + * >>> + * Description: >>> + * If the sga scatterlist is contiguous with the sgb scatterlist, >>> + * that means they can be merged together. >>> + * >>> + **/ >>> +static inline bool sg_is_contiguous(struct scatterlist *sga, >>> + struct scatterlist *sgb) >>> +{ >>> + return *(unsigned long *)sg_virt(sga) + sga->length == >>> + *(unsigned long *)sg_virt(sgb); >> As I already said, I don't like casts. > > OK. Could you give me a good example. Thanks. > >> >> But let's take some height : you're needing this function to decide to merge >> scatterlists. That means that you think the probability of having 2 scatterlist >> mergeable is not meaningless, ie. 50% or more. >> >> I suppose your scatterlists are allocated through kmalloc(). I'd like to know, >> through your testing, what is the success rate of sg_is_contiguous(), ie. I'd >> like to know how many times sg_is_contiguous() was called, and amongst these >> calls how many times it returned true. >> >> That will tell me how "worth" is this optimization. > > I think this is just one useful API for users, If the rate is only 1%, > we also need to check if they are contiguous to decide if they can be > coalesced. > >> >>> + * sg_add_sg_to_table - Add one scatterlist into sg table >>> + * @sgt: The sg table header to use >>> + * @src: The sg need to be added into sg table >>> + * >>> + * Description: >>> + * The 'nents' member indicates how many mapped scatterlists has been added >>> + * in the dynamic sg table. The 'orig_nents' member indicates the size of the >>> + * dynamic sg table. >>> + * >>> + * Copy one mapped @src@ scatterlist into the dynamic sg table and increase >>> + * 'nents' member. >>> + * >>> + **/ >> Okay, I still believe this one is wrong, because we don't understand each other. >> So let's take an example : >> sg_table = { >> .sgl = { >> { >> .page_link = PAGE_48, >> .offset = 0, >> .length = 2048, >> .dma_address = 0x30000, >> .dma_length = 4096, >> }, >> { >> .page_link = PAGE_48 | 0x02, >> .offset = 2048, >> .length = 2048, >> .dma_address = 0, >> .dma_length = 0, >> }, >> }, >> .nents = 1, >> .orig_nents = 2, >> }; >> >> In this example, by sheer luck the 2 scatterlist entries were physically >> contiguous, and the mapping function coallesced the 2 entries into only one >> (dma_address, dma_length) entry. That could also happen with an IOMMU by the >> way. Therefore, sg_table.nents = 1. >> >> If I understand your function correctly, it will erase sg_table.sgl[1], and that >> looks incorrect to me. This is why I can't understand how your code can be >> correct, and why I say you add a new "meaning" to sg_table->nents, which is not >> consistent with the meaning I understand. > > I think you misunderstood my point. The 'sg_add_sg_to_table()' > function is used to add one mapped scatterlist into the dynamic sg > table. For example: > 1. How to add one mapped sg into dynamic sg table > (1) If we allocate one dynamic sg table with 3 (small size can be > showed easily) empty scatterlists.: > sg_table = { > .sgl = { > { > .page_link = 0, > .offset = 0, > .length = 0, > }, > { > .page_link = 0, > .offset = 0, > .length = 0, > }, > { > .page_link = 0 | 0x02, > .offset = 0, > .length = 0, > }, > }, > .nents = 0, > .orig_nents = 3, > }; > > (2) If some module (like dm-crypt module) always sends one mapped > scatterlist (size is always 512bytes) to another module (crypto > driver) to handle the mapped scatterlist at one time. But we want to > collect the mapped scatterlist into one dynamic sg table to manage > them together, means we can send multiple mapped scatterlists (from > sg_table->sgl) to the crypto driver to handle them together to improve > its efficiency. So we add one mapped scatterlists into the dynamic sg > table. > mapped sg = { > .page_link = PAGE_20, > .offset = 0, > .length = 512, > }, > > Add into synamic sg table -------> > sg_table = { > .sgl = { > { > .page_link = PAGE_20 | 0x02, > .offset = 0, > .length = 512, > }, > { > .page_link = 0, > .offset = 0, > .length = 0, > }, > { > .page_link = 0, > .offset = 0, > .length = 0, > }, > }, > .nents = 1, > .orig_nents = 3, > }; > > Another two mapped scatterlists are added into synamic sg table -------> > sg_table = { > .sgl = { > { > .page_link = PAGE_20, > .offset = 0, > .length = 512, > }, > { > .page_link = PAGE_25, > .offset = 0, > .length = 512, > }, > { > .page_link = PAGE_28 | 0x20, > .offset = 0, > .length = 512, > }, > }, > .nents = 3, > .orig_nents = 3, > }; > > Then we can send the dynamic sg table to the crypto engine driver to > handle them together at one time. (If the dynamic sg table size is > 512, then the crypto engine driver can handle 512 scatterlists at one > time, which can improve much efficiency). That's the reason why we > want to introduce the dynamic sg table. > > 2. How to coalesce > (1) If one mapped scatterlsit already has been added into dynamic sg table: > sg_table = { > .sgl = { > { > .page_link = PAGE_20 | 0x02, > .offset = 0, > .length = 512, > }, > { > .page_link = 0, > .offset = 0, > .length = 0, > }, > { > .page_link = 0, > .offset = 0, > .length = 0, > }, > }, > .nents = 1, > .orig_nents = 3, > }; > > (2) Another mapped scatterlist comes. > mapped sg = { > .page_link = PAGE_20, > .offset = 512, > .length = 512, > }, > > So we check the new mapped scatterlist can be coalesced into previous > one in dynamic sg table like this: > sg_table = { > .sgl = { > { > .page_link = PAGE_20 | 0x02, > .offset = 0, > .length = 1024, > }, > { > .page_link = 0, > .offset = 0, > .length = 0, > }, > { > .page_link = 0, > .offset = 0, > .length = 0, > }, > }, > .nents = 1, > .orig_nents = 3, > }; > > It's done. We coalesced one scatterlist into antoher one to expand the > scatterlist's length. > Thanks for your comments. > It seems there are no more comments about this patchset? We really want to these APIs to coalesce scatterlists to improve the crypto engine efficiency. Thanks.
On Wed, Apr 20, 2016 at 03:34:47PM +0800, Baolin Wang wrote: > It seems there are no more comments about this patchset? We really > want to these APIs to coalesce scatterlists to improve the crypto > engine efficiency. Thanks. Your patch-set makes no sense. If the data can be coalesced then it shouldn't have been split up in the first place. I'm nacking the whole series. Cheers,
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h index 556ec1e..c1ed9f4 100644 --- a/include/linux/scatterlist.h +++ b/include/linux/scatterlist.h @@ -212,6 +212,20 @@ static inline void sg_unmark_end(struct scatterlist *sg) } /** + * sg_table_is_empty - Check if the sg table is empty + * @sgt: sg table + * + * Description: + * The 'orig_nents' member of one sg table is used to indicate how many + * scatterlists in the sg table. + * + **/ +static inline bool sg_table_is_empty(struct sg_table *sgt) +{ + return !sgt->orig_nents; +} + +/** * sg_phys - Return physical address of an sg entry * @sg: SG entry * @@ -241,6 +255,23 @@ static inline void *sg_virt(struct scatterlist *sg) return page_address(sg_page(sg)) + sg->offset; } +/** + * sg_is_contiguous - Check if the scatterlists are contiguous + * @sga: SG entry + * @sgb: SG entry + * + * Description: + * If the sga scatterlist is contiguous with the sgb scatterlist, + * that means they can be merged together. + * + **/ +static inline bool sg_is_contiguous(struct scatterlist *sga, + struct scatterlist *sgb) +{ + return *(unsigned long *)sg_virt(sga) + sga->length == + *(unsigned long *)sg_virt(sgb); +} + int sg_nents(struct scatterlist *sg); int sg_nents_for_len(struct scatterlist *sg, u64 len); struct scatterlist *sg_next(struct scatterlist *); @@ -261,6 +292,8 @@ void sg_free_table(struct sg_table *); int __sg_alloc_table(struct sg_table *, unsigned int, unsigned int, struct scatterlist *, gfp_t, sg_alloc_fn *); int sg_alloc_table(struct sg_table *, unsigned int, gfp_t); +int sg_alloc_empty_table(struct sg_table *, unsigned int, gfp_t); +int sg_add_sg_to_table(struct sg_table *, struct scatterlist *); int sg_alloc_table_from_pages(struct sg_table *sgt, struct page **pages, unsigned int n_pages, unsigned long offset, unsigned long size, diff --git a/lib/scatterlist.c b/lib/scatterlist.c index 004fc70..6d3f3b0 100644 --- a/lib/scatterlist.c +++ b/lib/scatterlist.c @@ -370,6 +370,75 @@ int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask) EXPORT_SYMBOL(sg_alloc_table); /** + * sg_add_sg_to_table - Add one scatterlist into sg table + * @sgt: The sg table header to use + * @src: The sg need to be added into sg table + * + * Description: + * The 'nents' member indicates how many mapped scatterlists has been added + * in the dynamic sg table. The 'orig_nents' member indicates the size of the + * dynamic sg table. + * + * Copy one mapped @src@ scatterlist into the dynamic sg table and increase + * 'nents' member. + * + **/ +int sg_add_sg_to_table(struct sg_table *sgt, struct scatterlist *src) +{ + unsigned int i = 0, orig_nents = sgt->orig_nents; + struct scatterlist *sgl = sgt->sgl; + struct scatterlist *sg; + + /* Check if there are enough space for the new sg to be added */ + if (sgt->nents >= sgt->orig_nents) + return -EINVAL; + + for_each_sg(sgl, sg, orig_nents, i) { + if (sgt->nents > 0 && i == (sgt->nents - 1)) { + sg_unmark_end(sg); + } else if (i == sgt->nents) { + memcpy(sg, src, sizeof(struct scatterlist)); + sg_mark_end(sg); + sgt->nents++; + break; + } + } + + return 0; +} + +/** + * sg_alloc_empty_table - Allocate one empty dynamic sg table + * @sgt: The sg table header to use + * @nents: Number of entries in sg list + * @gfp_mask: GFP allocation mask + * + * Description: + * Allocate and initialize one dynamic sg table. One dynamic sg table means + * it need allocate @nents@ empty scatterlists entries and is used to copy + * other mapped scatterlists into the dynamic sg table. + * + * The 'nents' member indicates how many scatterlists has been copied into + * the dynamic sg table. It should set 0 which means there are no mapped + * scatterlists added in this sg table now. + * + * The 'orig_nents' member indicates the size of the dynamic sg table. + * + **/ +int sg_alloc_empty_table(struct sg_table *sgt, unsigned int nents, + gfp_t gfp_mask) +{ + int ret; + + ret = sg_alloc_table(sgt, nents, gfp_mask); + if (ret) + return ret; + + sgt->nents = 0; + return 0; +} + +/** * sg_alloc_table_from_pages - Allocate and initialize an sg table from * an array of pages * @sgt: The sg table header to use
In crypto engine framework, one request can combine (copy) other requests' scatterlists into its dynamic sg table to manage them together as a bulk block , which can improve engine efficency with handling bulk block. Thus we need some helper functions to manage dynamic scattertables. This patch introduces 'sg_is_contiguous()' function to check if two scatterlists are contiguous, 'sg_alloc_empty_table()' function to allocate one empty dynamic sg table, 'sg_add_sg_to_table()' function to copy one mapped scatterlist into sg table and 'sg_table_is_empty' function to check if the sg table is empty. Signed-off-by: Baolin Wang <baolin.wang@linaro.org> --- include/linux/scatterlist.h | 33 +++++++++++++++++++++ lib/scatterlist.c | 69 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+)