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

[Baolin Wang]

Hi Robert,

On 5 April 2016 at 15:10, Baolin Wang <baolin.wang@xxxxxxxxxx> wrote:
> Hi Robert,
>
> Sorry for the late reply.
>
> On 2 April 2016 at 23:00, Robert Jarzmik <robert.jarzmik@xxxxxxx> wrote:
>> Baolin Wang <baolin.wang@xxxxxxxxxx> 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.

-- 
Baolin.wang
Best Regards