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

[Daniele Palmas <dnlplm@gmail.com>]

Hello Subash,

Il giorno ven 11 nov 2022 alle ore 02:17 Subash Abhinov
Kasiviswanathan (KS) <quic_subashab@quicinc.com> ha scritto:
>
> 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);
>

Sure, I'll test that on Monday.

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

Ack.

Thanks,
Daniele

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