From: "Subash Abhinov Kasiviswanathan (KS)" <quic_subashab@quicinc.com>
To: Alexander Lobakin <alexandr.lobakin@intel.com>,
Daniele Palmas <dnlplm@gmail.com>
Cc: "David Miller" <davem@davemloft.net>,
"Jakub Kicinski" <kuba@kernel.org>,
"Paolo Abeni" <pabeni@redhat.com>,
"Eric Dumazet" <edumazet@google.com>,
"Sean Tranchetti" <quic_stranche@quicinc.com>,
"Jonathan Corbet" <corbet@lwn.net>, "Bjørn Mork" <bjorn@mork.no>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
netdev@vger.kernel.org
Subject: Re: [PATCH net-next 2/3] net: qualcomm: rmnet: add tx packets aggregation
Date: Thu, 10 Nov 2022 18:17:09 -0700 [thread overview]
Message-ID: <b84e45e0-55e0-a1f5-e1cc-980983946019@quicinc.com> (raw)
In-Reply-To: <20221110173222.3536589-1-alexandr.lobakin@intel.com>
On 11/10/2022 10:32 AM, Alexander Lobakin wrote:
> From: Daniele Palmas <dnlplm@gmail.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@codeaurora.org>
>> Sean Tranchetti <stranche@codeaurora.org>
>>
>> Signed-off-by: Daniele Palmas <dnlplm@gmail.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
next prev parent reply other threads:[~2022-11-11 1:18 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-09 18:02 [PATCH net-next 0/3] add tx packets aggregation to ethtool and rmnet Daniele Palmas
2022-11-09 18:02 ` [PATCH net-next 1/3] ethtool: add tx aggregation parameters Daniele Palmas
2022-11-11 17:07 ` Jakub Kicinski
2022-11-11 21:51 ` Daniele Palmas
2022-11-13 9:48 ` Gal Pressman
2022-11-14 10:06 ` Daniele Palmas
2022-11-14 10:45 ` Dave Taht
2022-11-15 11:51 ` Daniele Palmas
2022-11-15 15:27 ` Dave Taht
2022-11-15 0:42 ` Jakub Kicinski
2022-11-15 10:59 ` Gal Pressman
2022-11-15 16:21 ` Jakub Kicinski
2022-11-09 18:02 ` [PATCH net-next 2/3] net: qualcomm: rmnet: add tx packets aggregation Daniele Palmas
2022-11-10 17:32 ` Alexander Lobakin
2022-11-11 1:17 ` Subash Abhinov Kasiviswanathan (KS) [this message]
2022-11-11 17:11 ` Jakub Kicinski
2022-11-11 22:00 ` Daniele Palmas
2022-11-14 8:48 ` Daniele Palmas
2022-11-11 17:23 ` Daniele Palmas
2022-11-16 15:19 ` Daniele Palmas
2022-11-16 16:20 ` Alexander Lobakin
2022-11-20 9:25 ` Daniele Palmas
2022-11-20 9:39 ` Bjørn Mork
2022-11-20 9:52 ` Daniele Palmas
2022-11-20 17:48 ` Subash Abhinov Kasiviswanathan (KS)
2022-11-21 7:00 ` Daniele Palmas
2022-11-24 3:32 ` Subash Abhinov Kasiviswanathan (KS)
2022-11-10 19:09 ` kernel test robot
2022-11-11 17:14 ` Jakub Kicinski
2022-11-14 9:13 ` Daniele Palmas
2022-11-14 10:25 ` Bjørn Mork
2022-11-15 0:43 ` Jakub Kicinski
2022-11-09 18:02 ` [PATCH net-next 3/3] net: qualcomm: rmnet: add ethtool support for configuring tx aggregation Daniele Palmas
2022-11-11 6:46 ` kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b84e45e0-55e0-a1f5-e1cc-980983946019@quicinc.com \
--to=quic_subashab@quicinc.com \
--cc=alexandr.lobakin@intel.com \
--cc=bjorn@mork.no \
--cc=corbet@lwn.net \
--cc=davem@davemloft.net \
--cc=dnlplm@gmail.com \
--cc=edumazet@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=quic_stranche@quicinc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).