netdev - Re: [PATCH net-next 2/3] net: qualcomm: rmnet: add tx packets aggregation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87tu31hlyb.fsf@miraculix.mork.no>
Date:   Mon, 14 Nov 2022 11:25:32 +0100
From:   Bjørn Mork <bjorn@...k.no>
To:     Daniele Palmas <dnlplm@...il.com>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Subash Abhinov Kasiviswanathan <quic_subashab@...cinc.com>,
        Sean Tranchetti <quic_stranche@...cinc.com>,
        Jonathan Corbet <corbet@....net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/3] net: qualcomm: rmnet: add tx packets
 aggregation

Daniele Palmas <dnlplm@...il.com> writes:
> Il giorno ven 11 nov 2022 alle ore 18:14 Jakub Kicinski
> <kuba@...nel.org> ha scritto:
>>
>> On Wed,  9 Nov 2022 19:02:48 +0100 Daniele Palmas wrote:
>> > +bool rmnet_map_tx_agg_skip(struct sk_buff *skb)
>> > +{
>> > +     bool is_icmp = 0;
>> > +
>> > +     if (skb->protocol == htons(ETH_P_IP)) {
>> > +             struct iphdr *ip4h = ip_hdr(skb);
>> > +
>> > +             if (ip4h->protocol == IPPROTO_ICMP)
>> > +                     is_icmp = true;
>> > +     } else if (skb->protocol == htons(ETH_P_IPV6)) {
>> > +             unsigned int icmp_offset = 0;
>> > +
>> > +             if (ipv6_find_hdr(skb, &icmp_offset, IPPROTO_ICMPV6, NULL, NULL) == IPPROTO_ICMPV6)
>> > +                     is_icmp = true;
>> > +     }
>> > +
>> > +     return is_icmp;
>> > +}
>>
>> Why this? I don't see it mention in the commit message or any code
>> comment.
>
> This is something I've found in downstream code: with my test setup
> and scenario it does not make any difference on the icmp packets
> timing (both with or without throughput tests ongoing), but I don't
> have access to all the systems for which rmnet is used.
>
> So, I'm not sure if it solves a real issue in other situations.
>
> I can move that out and me or someone else will add it again in case
> there will be a real issue to be solved.

It looks like an attempt to "cheat" latency measurements.  I don't think
we should do that.  Aggregation may be necessary to achieve maximum
throughput in a radio network, but has its obvious bufferbloat downside.
Let's not hide that fact.  Users deserve to know, and tune their systems
accordingly.  Things like this will only make that more difficult


Bjørn