Re: [PATCH 1/4] scatterlist: Introduce some helper functions

2016-03-10 Thread Baolin Wang 
On 10 March 2016 at 17:42, Robert Jarzmik  wrote:
>>
>>
>> Ah, sorry that's a mistake. It should check as below:
>> static inline bool sg_is_contiguous(struct scatterlist *sga, struct
>> scatterlist *sgb)
>> {
>> return (unsigned int)sg_virt(sga) + sga->length == (unsigned
>> int)sg_virt(sgb);
>> }
> NAK.
> First, I don't like the cast.
> Second ask yourself: what if you compile on a 64 bit architecture ?

Sorry, it's a mistake. I've resend one email to correct that with
'unsigned long'.

> Third, I don't like void* arithmetics, some compilers might refuse it.

OK. I will modify that.

>
> And as a last comment, is it "virtual" contiguity you're looking after, 
> physical
> contiguity, or dma_addr_t contiguity ?

Yes, I need check the virtual contiguity.

>
 @@ -370,6 +370,65 @@ int sg_alloc_table(struct sg_table *table, unsigned 
 int
 nents, gfp_t gfp_mask)
>>> ...
  /**
 + * 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 scatterlists added in the sg 
 table.
 + *   Copy the @src@ scatterlist into 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;
>>> I must admit I don't understand that one either : how do comparing the 
>>> number of
>>> "mapped" entries against the number of "allocated" entries determines if 
>>> there
>>> is enough room ?
>>
>> That's for a dynamic sg table. If there is one sg table allocated
>> 'orig_nents' scatterlists, and we need copy another mapped scatterlist
>> into the sg table if there are some requirements. So we use 'nents' to
>> record how many scatterlists have been copied into the sg table.
> As I'm still having difficulities to understand, please explain to me :
>  - what is a dynamic sg_table

OK. That's for some special usage. For example, the dm-crypt will send
one request mapped one scatterlist to engine level to handle the
request at one time. But we want to collect some requests together to
handle together at one time to improve engine efficiency. So in crypto
engine layer we need to copy the mapped scatterlists into one sg
table, which has been allocated some sg memory.

>  - how it works

The 'orig_nents' means how many scatterlists the sg table can hold.
The 'nents' means how many scatterlists have been copied into sg
table. So the 'nents' will be not equal as 'orig_nents' unless the sg
table is full.

>  - if it's "struct sg_table", how the field of this structure are different 
> when
>it's a dynamic sg_table from a "static sg_table" ?

>
 +/**
 + * sg_alloc_empty_table - Allocate one empty 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 an sg table. The 'nents' member of sg_table
 + *indicates how many scatterlists added in the sg table. It should set
 + *0 which means there are no scatterlists added in this sg table now.
 + *
 + **/
 +int sg_alloc_empty_table(struct sg_table *sgt, unsigned int nents,
 +  gfp_t gfp_mask)
>>> As for this one, there has to be a purpose for it I fail to see. From far 
>>> away
>>> it looks exactly like sg_alloc_table(), excepting it "works around" the 
>>> nents >
>>> 0 protection of __sg_alloc_table().
>>> What is exactly the need for this one, and if it's usefull why not simply
>>> changing the __sg_alloc_table() "nents > 0" test and see what the outcome 
>>> of the
>>> review will be ?
>>
>> Like I said above. If we want to copy some mapped scatterlists into
>> one sg table, we should set the 'nents' to 0 to indicates how many
>> scatterlists coppied in the sg table.
> So how do you unmap this scatterlist once you've lost the number of mapped
> entries ?

I will not lost the number of mapped in sg table,  'nents' member
means how many mapped scatterlists in the dynamic table. Every time
one mapped sg copied into the dynamic table, the 'nents' will increase
1.

>
> I think we'll come back to this once I understand how a "dynamic" sg_table is
> different from a "static" one.
>
> Cheers.
>
> --
> Robert



-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] scatterlist: Introduce some helper functions

2016-03-10 Thread Robert Jarzmik 
Baolin Wang  writes:

> Hi Robert,
>
> On 4 March 2016 at 03:15, Robert Jarzmik  wrote:
>> Baolin Wang  writes:
>>> +static inline bool sg_is_contiguous(struct scatterlist *sga,
>>> + struct scatterlist *sgb)
>>> +{
>>> + return ((sga->page_link & ~0x3UL) + sga->offset + sga->length ==
>>> + (sgb->page_link & ~0x3UL));
>>> +}
>> I don't understand that one.
>> sga->page_link is a pointer to a "struct page *". How can it be added to an
>> offset within a page ???
>
>
> Ah, sorry that's a mistake. It should check as below:
> static inline bool sg_is_contiguous(struct scatterlist *sga, struct
> scatterlist *sgb)
> {
> return (unsigned int)sg_virt(sga) + sga->length == (unsigned
> int)sg_virt(sgb);
> }
NAK.
First, I don't like the cast.
Second ask yourself: what if you compile on a 64 bit architecture ?
Third, I don't like void* arithmetics, some compilers might refuse it.

And as a last comment, is it "virtual" contiguity you're looking after, physical
contiguity, or dma_addr_t contiguity ?

>>> @@ -370,6 +370,65 @@ int sg_alloc_table(struct sg_table *table, unsigned int
>>> nents, gfp_t gfp_mask)
>> ...
>>>  /**
>>> + * 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 scatterlists added in the sg 
>>> table.
>>> + *   Copy the @src@ scatterlist into 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;
>> I must admit I don't understand that one either : how do comparing the 
>> number of
>> "mapped" entries against the number of "allocated" entries determines if 
>> there
>> is enough room ?
>
> That's for a dynamic sg table. If there is one sg table allocated
> 'orig_nents' scatterlists, and we need copy another mapped scatterlist
> into the sg table if there are some requirements. So we use 'nents' to
> record how many scatterlists have been copied into the sg table.
As I'm still having difficulities to understand, please explain to me :
 - what is a dynamic sg_table
 - how it works
 - if it's "struct sg_table", how the field of this structure are different when
   it's a dynamic sg_table from a "static sg_table" ?

>>> +/**
>>> + * sg_alloc_empty_table - Allocate one empty 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 an sg table. The 'nents' member of sg_table
>>> + *indicates how many scatterlists added in the sg table. It should set
>>> + *0 which means there are no scatterlists added in this sg table now.
>>> + *
>>> + **/
>>> +int sg_alloc_empty_table(struct sg_table *sgt, unsigned int nents,
>>> +  gfp_t gfp_mask)
>> As for this one, there has to be a purpose for it I fail to see. From far 
>> away
>> it looks exactly like sg_alloc_table(), excepting it "works around" the 
>> nents >
>> 0 protection of __sg_alloc_table().
>> What is exactly the need for this one, and if it's usefull why not simply
>> changing the __sg_alloc_table() "nents > 0" test and see what the outcome of 
>> the
>> review will be ?
>
> Like I said above. If we want to copy some mapped scatterlists into
> one sg table, we should set the 'nents' to 0 to indicates how many
> scatterlists coppied in the sg table.
So how do you unmap this scatterlist once you've lost the number of mapped
entries ?

I think we'll come back to this once I understand how a "dynamic" sg_table is
different from a "static" one.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] scatterlist: Introduce some helper functions

2016-03-03 Thread Baolin Wang 
Hi Robert,

On 4 March 2016 at 03:15, Robert Jarzmik  wrote:
> Baolin Wang  writes:
>
>> @@ -212,6 +212,37 @@ static inline void sg_unmark_end(struct scatterlist *sg)
>>  }
>>
>>  /**
>> + * 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 ((sga->page_link & ~0x3UL) + sga->offset + sga->length ==
>> + (sgb->page_link & ~0x3UL));
>> +}
> I don't understand that one.
> sga->page_link is a pointer to a "struct page *". How can it be added to an
> offset within a page ???


Ah, sorry that's a mistake. It should check as below:
static inline bool sg_is_contiguous(struct scatterlist *sga, struct
scatterlist *sgb)
{
return (unsigned int)sg_virt(sga) + sga->length == (unsigned
int)sg_virt(sgb);
}

>
>> @@ -370,6 +370,65 @@ int sg_alloc_table(struct sg_table *table, unsigned int
>> nents, gfp_t gfp_mask)
> ...
>>  /**
>> + * 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 scatterlists added in the sg 
>> table.
>> + *   Copy the @src@ scatterlist into 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;
> I must admit I don't understand that one either : how do comparing the number 
> of
> "mapped" entries against the number of "allocated" entries determines if there
> is enough room ?

That's for a dynamic sg table. If there is one sg table allocated
'orig_nents' scatterlists, and we need copy another mapped scatterlist
into the sg table if there are some requirements. So we use 'nents' to
record how many scatterlists have been copied into the sg table.

>
>> +/**
>> + * sg_alloc_empty_table - Allocate one empty 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 an sg table. The 'nents' member of sg_table
>> + *indicates how many scatterlists added in the sg table. It should set
>> + *0 which means there are no scatterlists added in this sg table now.
>> + *
>> + **/
>> +int sg_alloc_empty_table(struct sg_table *sgt, unsigned int nents,
>> +  gfp_t gfp_mask)
> As for this one, there has to be a purpose for it I fail to see. From far away
> it looks exactly like sg_alloc_table(), excepting it "works around" the nents 
> >
> 0 protection of __sg_alloc_table().
> What is exactly the need for this one, and if it's usefull why not simply
> changing the __sg_alloc_table() "nents > 0" test and see what the outcome of 
> the
> review will be ?

Like I said above. If we want to copy some mapped scatterlists into
one sg table, we should set the 'nents' to 0 to indicates how many
scatterlists coppied in the sg table.
Thanks for your comments.

>
> Cheers.
>
> --
> Robert



-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] scatterlist: Introduce some helper functions

2016-03-03 Thread Robert Jarzmik 
Baolin Wang  writes:

> @@ -212,6 +212,37 @@ static inline void sg_unmark_end(struct scatterlist *sg)
>  }
>  
>  /**
> + * 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 ((sga->page_link & ~0x3UL) + sga->offset + sga->length ==
> + (sgb->page_link & ~0x3UL));
> +}
I don't understand that one.
sga->page_link is a pointer to a "struct page *". How can it be added to an
offset within a page ???

> @@ -370,6 +370,65 @@ int sg_alloc_table(struct sg_table *table, unsigned int
> nents, gfp_t gfp_mask)
...
>  /**
> + * 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 scatterlists added in the sg 
> table.
> + *   Copy the @src@ scatterlist into 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;
I must admit I don't understand that one either : how do comparing the number of
"mapped" entries against the number of "allocated" entries determines if there
is enough room ?

> +/**
> + * sg_alloc_empty_table - Allocate one empty 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 an sg table. The 'nents' member of sg_table
> + *indicates how many scatterlists added in the sg table. It should set
> + *0 which means there are no scatterlists added in this sg table now.
> + *
> + **/
> +int sg_alloc_empty_table(struct sg_table *sgt, unsigned int nents,
> +  gfp_t gfp_mask)
As for this one, there has to be a purpose for it I fail to see. From far away
it looks exactly like sg_alloc_table(), excepting it "works around" the nents >
0 protection of __sg_alloc_table().
What is exactly the need for this one, and if it's usefull why not simply
changing the __sg_alloc_table() "nents > 0" test and see what the outcome of the
review will be ?

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] scatterlist: Introduce some helper functions

2016-03-02 Thread Baolin Wang 
In crypto engine framework, one request can combine other requests'
scatterlists into its sg table to 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 sg table, 'sg_add_sg_to_table()' function to add
one scatterlist into sg table and 'sg_table_is_empty' function to
check if the sg table is empty.

Signed-off-by: Baolin Wang 
---
 include/linux/scatterlist.h |   33 
 lib/scatterlist.c   |   59 +++
 2 files changed, 92 insertions(+)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 556ec1e..ae9aee6 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -212,6 +212,37 @@ static inline void sg_unmark_end(struct scatterlist *sg)
 }
 
 /**
+ * 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 ((sga->page_link & ~0x3UL) + sga->offset + sga->length ==
+   (sgb->page_link & ~0x3UL));
+}
+
+/**
+ * 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
  *
@@ -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..e9e5d5a 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -370,6 +370,65 @@ 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 scatterlists added in the sg table.
+ *   Copy the @src@ scatterlist into 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 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 an sg table. The 'nents' member of sg_table
+ *indicates how many scatterlists added in the sg table. It should set
+ *0 which means there are no scatterlists added in this sg table now.
+ *
+ **/
+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
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html