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

[Felix Fietkau <nbd@openwrt.org>]

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);

>>> 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