Re: [PATCH] ath10k: add modparam 'hw_csum' to make HW checksum configurable

[Peter Oh <poh@codeaurora.org>]

On 12/16/2015 01:54 PM, Felix Fietkau wrote:
> On 2015-12-16 22:19, Peter Oh wrote:
>> On 12/16/2015 12:53 PM, Felix Fietkau wrote:
>>> On 2015-12-16 21:46, Peter Oh wrote:
>>>> On 12/16/2015 12:35 PM, Felix Fietkau wrote:
>>>>> On 2015-12-16 21:29, Peter Oh wrote:
>>>>>> On 12/16/2015 10:27 AM, Felix Fietkau wrote:
>>>>>>> On 2015-12-16 19:20, Peter Oh wrote:
>>>>>>>> Some hardwares such as QCA988X and QCA99X0 doesn't have
>>>>>>>> capability of checksum offload when frame formats are not
>>>>>>>> suitable for it such as Mesh frame.
>>>>>>>> Hence add a module parameter, hw_csum, to make checksum offload
>>>>>>>> configurable during module registration time.
>>>>>>>>
>>>>>>>> Signed-off-by: Peter Oh <poh@qca.qualcomm.com>
>>>>>>> How about instead of inventing yet another crappy module parameter, you
>>>>>>> call skb_checksum_help() in the driver in cases where the hardware is
>>>>>>> unable to offload the checksum calculation.
>>>>>>>
>>>>>>> That way the user has to worry about less driver specific hackery ;)
>>>>>> That will be good option for hardware not supporting HW checksum, but I
>>>>>> mind that using the function will add more workload per every packet on
>>>>>> critical data path when HW supports checksum resulting in throughput down.
>>>>> I didn't mean calling it for every single frame in the data path.
>>>>> What I'm suggesting is calling it selectively only for mesh frames, or
>>>>> any other frames that the hardware cannot offload, and leaving the rest
>>>>> for the hardware to process.
>>>>>
>>>>> There should be no performance difference between disabling checksum
>>>>> offload and calling skb_checksum_help from the driver.
>>>> To call it selectively for Mesh frame or interface, we need to add it on
>>>> mac80211 layer such as ieee80211_build_hdr() since driver layer does not
>>>> care the interface type in data path.
>>> No need to change mac80211 - it only touches the headers, and
>>> skb_checksum_help does not care about that. The skb has enough
>>> information for it to find the right range to calculate the checksum and
>>> the place to store it.
>> If mentioned to use the function to mesh frame only without touching
>> mac80211, then how do you suggest it to apply it only to mesh frame
>> without interfere other data frames?
>> Can you share your example?
> It's trivial - in ath10k_tx you do this:
>
> if (vif->type == NL80211_IFTYPE_MESH_POINT &&
>      skb->ip_summed == CHECKSUM_PARTIAL)
> 	skb_checksum_help(skb);
Thank you Felix for the quick response.
I agree on your user experience opinion,
but what do you think when ath10k has a new chip supporting HW checksum 
for Mesh?
>>>> In that case it will also introduce throughput degrade to HW that
>>>> supports HW checksum for Mesh.
>>> This doesn't make any sense to me. Are you saying that there's no way
>>> for the driver to detect the cases where the hardware cannot do checksum
>>> offloading?
>> I'm saying the case that HW supports checksum except for specific frame
>> such as Mesh and to make driver support both case dynamically at code
>> level, it requires extra codes which need to check if the frame is Mesh
>> or not. Since this approach requires extra workload especially in data
>> path, it will degrade driver's performance.
> The check is cheap enough that it will not have any visible impact. And
> the improved user experience is certainly worth it ;)
>
> - Felix
Thanks,
Peter