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

From: Franklin S Cooper Jr.
Date: Wed Jul 06 2016 - 15:40:52 EST




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.