Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

[Franklin S Cooper Jr.]

On 07/06/2016 12:46 PM, Andy Shevchenko wrote:
> On Wed, Jul 6, 2016 at 8:09 PM, Franklin S Cooper Jr. <fcooper@xxxxxx> wrote:
> 
>>>> +               last_entry = sg_is_last(sgl);
>>>> +               *sgl = *orig_sgl;
>>>> +
>>>> +               /*
>>>> +                * If page_link is pointing to a chained sgl then set it to
>>>> +                * zero since we already compensated for chained sgl. If
>>>> +                * page_link is pointing to a page then clear bits 1 and 0.
>>>> +                */
>>>> +               if (sg_is_chain(orig_sgl))
>>>> +                       sgl->page_link = 0x0;
>>>> +               else
>>>> +                       sgl->page_link &= ~0x03;
>>>> +
>>>
>>>> +               if (last_entry) {
>>>> +                       sg_dma_len(sgl) = len;
>>>
>>> Hmm... If it's not DMA mapped and CONFIG_NEED_SG_DMA_LENGTH=y, you
>>> will set dma_length field and len otherwise. Is it on purpose?
>>
>> Thanks for pointing this out. I didn't take into consideration the above
>> scenario.
>>
>> After looking at this more it seems like on occasion dma_length !=
>> length. I see this occurring in various map_sg functions for several
>> architectures.
>>
>> I'm simply trying to reduce the overall sgl length by a certain amount.
>> I'm not sure what the proper approach would be since dma_length and
>> length can be set to different values. Simply setting sgl->dma_length to
>> sgl->length or vice a versa seems like it can be problematic in some
>> scenarios. What confuses me even more is if dma_length != length then
>> how can sg_nents_for_len only use the sgl length property.
>>
>> Any thoughts on what would be the best way to handle this?
> 
> First come to my mind is to refuse to clone if SG table is DMA mapped.
> 
> Imagine the scenario where you have for example last entry which your
> caller asked to split as
> 
> total_entry_len = 37235 (input)
> dma_length = 65536 (64k alignment)


Do you know of an example where dma_length will be set to some kind of
alignment size rather than based on the number of bytes you want
transfered? Or maybe I'm miss understanding this.

> 
> asked_len = 32768 (Split chunks: 32768 + 4467)
> resulting_dma_len = ?
> 
> What do you put here? Initially it perhaps means the same 64k. But how
> to distinguish?
> (I couldn't come with better one, but there are many alike scenarios)
> 
> Only hack we have is to supply struct device of the DMA device to this
> SG table where you can get max segment size (but I dunno about
> alignment).
> 
> P.S. I'm not familiar of the details of all these.
> 

Unfortunately, for the original purpose of this series the scatterlist I
want to clone is indeed DMA mapped.

The times I've seen dma_length != length is when the dma_map_sg is
trying to combine contiguous sgl (ex ppc_iommu_map_sg and dma_map_cont).
So maybe it is better to subtract a value from length and dma_length
rather than explicitly setting it to a value. This way it wouldn't
matter that dma_length and length aren't equal.

This looks like a similar approach used by sg_split_mapped. Although
sg_split skips first x number of bytes while I want to skip the last x
number of bytes.

Maybe Robert can add his thoughts since he created sg_split.