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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Thu, 10 Nov 2022 18:17:09 -0700
From:   "Subash Abhinov Kasiviswanathan (KS)" <quic_subashab@...cinc.com>
To:     Alexander Lobakin <alexandr.lobakin@...el.com>,
        Daniele Palmas <dnlplm@...il.com>
CC:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        "Sean Tranchetti" <quic_stranche@...cinc.com>,
        Jonathan Corbet <corbet@....net>,
        Bjørn Mork <bjorn@...k.no>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 2/3] net: qualcomm: rmnet: add tx packets
 aggregation

On 11/10/2022 10:32 AM, Alexander Lobakin wrote:
> From: Daniele Palmas <dnlplm@...il.com>
> Date: Wed,  9 Nov 2022 19:02:48 +0100
> 
>> Bidirectional TCP throughput tests through iperf with low-cat
>> Thread-x based modems showed performance issues both in tx
>> and rx.
>>
>> The Windows driver does not show this issue: inspecting USB
>> packets revealed that the only notable change is the driver
>> enabling tx packets aggregation.
>>
>> Tx packets aggregation, by default disabled, requires flag
>> RMNET_FLAGS_EGRESS_AGGREGATION to be set (e.g. through ip command).
>>
>> The maximum number of aggregated packets and the maximum aggregated
>> size are by default set to reasonably low values in order to support
>> the majority of modems.
>>
>> This implementation is based on patches available in Code Aurora
>> repositories (msm kernel) whose main authors are
>>
>> Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>
>> Sean Tranchetti <stranche@...eaurora.org>
>>
>> Signed-off-by: Daniele Palmas <dnlplm@...il.com>
>> ---
>>   
>> +struct rmnet_egress_agg_params {
>> +	u16 agg_size;
> 
> skbs can now be way longer than 64 Kb.
> 

rmnet devices generally use a standard MTU (around 1500) size.
Would it still be possible for >64kb to be generated as no relevant 
hw_features is set for large transmit offloads.
Alternatively, are you referring to injection of packets explicitly, say 
via packet sockets.

>> +	u16 agg_count;
>> +	u64 agg_time_nsec;
>> +};
>> +
> Do I get the whole logics correctly, you allocate a new big skb and
> just copy several frames into it, then send as one chunk once its
> size reaches the threshold? Plus linearize every skb to be able to
> do that... That's too much of overhead I'd say, just handle S/G and
> fraglists and make long trains of frags from them without copying
> anything? Also BQL/DQL already does some sort of aggregation via
> ::xmit_more, doesn't it? Do you have any performance numbers?

The difference here is that hardware would use a single descriptor for 
aggregation vs multiple descriptors for scatter gather.

I wonder if this issue is related to pacing though.
Daniele, perhaps you can try this hack without enabling EGRESS 
AGGREGATION and check if you are able to reach the same level of 
performance for your scenario.

--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
@@ -236,7 +236,7 @@ void rmnet_egress_handler(struct sk_buff *skb)
         struct rmnet_priv *priv;
         u8 mux_id;

-       sk_pacing_shift_update(skb->sk, 8);
+       skb_orphan(skb);

         orig_dev = skb->dev;
         priv = netdev_priv(orig_dev);

> 
>> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>> index 5e7a1041df3a..09a30e2b29b1 100644
>> --- a/include/uapi/linux/if_link.h
>> +++ b/include/uapi/linux/if_link.h
>> @@ -1351,6 +1351,7 @@ enum {
>>   #define RMNET_FLAGS_EGRESS_MAP_CKSUMV4            (1U << 3)
>>   #define RMNET_FLAGS_INGRESS_MAP_CKSUMV5           (1U << 4)
>>   #define RMNET_FLAGS_EGRESS_MAP_CKSUMV5            (1U << 5)
>> +#define RMNET_FLAGS_EGRESS_AGGREGATION            (1U << 6)
> 
> But you could rely on the aggregation parameters passed via Ethtool
> to decide whether to enable aggregation or not. If any of them is 0,
> it means the aggregation needs to be disabled.
> Otherwise, to enable it you need to use 2 utilities: the one that
> creates RMNet devices at first and Ethtool after, isn't it too
> complicated for no reason?

Yes, the EGRESS AGGREGATION parameters can be added as part of the rmnet 
netlink policies.

> 
>>   
>>   enum {
>>   	IFLA_RMNET_UNSPEC,
>> -- 
>> 2.37.1
> 
> Thanks,
> Olek