Re: [PATCH net-next v2] ethtool: ice: Support for RSS settings to GTP from ethtool

[takeru hayasaka]

Hi Harald-san

Thanks for your review!

> so if I'm guessing correctly, those would be hashing only on the V4/V6
destination address?  Why would that be GTP specific?  The IPv4/v6
header in front of the GTP header is a normal IP header.

This is not correct. The TEID and the src port/dst port of the inner
packet are also included.

> Are there really deployments where the *very limited* GTP-C control
I also think that it should not be limited to GTP-C. However, as I
wrote in the email earlier, all the flows written are different in
packet structure, including GTP-C. In the semantics of ethtool, I
thought it was correct to pass a fixed packet structure and the
controllable parameters for it. At least, the Intel ice driver that I
modified is already like that.

> IMHO that kind of explanation should be in the comment next to the
> #define (for all of them) rather than "hash only".  That way it's
> obvious to the reader what they do, rather than having to guess.

Regarding what should be hashed, this is a complex case. It will also
answer other questions, but for example, if you read this Intel ice
driver, there are cases where you can manipulate the port of the Inter
packet. I think this varies depending on the driver to be implemented.

Note that these comments follow the existing code of ethtool.

FYI: I think it will be helpful for you!
https://www.intel.com/content/www/us/en/content-details/617015/intel-ethernet-controller-e810-dynamic-device-personalization-ddp-technology-guide.html
(cf. Table 8. Patterns and Input Sets for iavf RSS)

2023年10月17日(火) 23:18 takeru hayasaka <hayatake396@xxxxxxxxx>:
>
> Hi Jakub-san and Simon-san
> Thank you for reviewing again!
>
> > Reviewed-by: Simon Horman <horms@xxxxxxxxxx>
> Thanks;)
>
> > Adding Willem, Pablo, and Harald to CC (please CC them on future
> versions).
>
> of course.  thanks!
>
> > nit: please note that these are hex numbers,
>      next value after 0x19 is 0x1a, not 0x20.
>
> !!!!! I'm so embarrassed.... I will next version fix
>
> > What gives me pause here is the number of flow sub-types we define
> > for GTP hashing.
> >
> > My understanding of GTP is limited to what I just read on Wikipedia.
> >
> > IIUC the GTPC vs GTPU distinction comes down to the UDP port on
> > which the protocol runs? Are the frames also different?
> >
> > I'm guessing UL/DL are uplink/downlink but what's EH?
> >
> > How do GTPU_V4_FLOW, GTPU_EH_V4_FLOW, GTPU_UL_V4_FLOW, and
> > GTPU_DL_V4_FLOW differ?
> >
> > Key question is - are there reasonable use cases that you can think of
> > for enabling GTP hashing for each one of those bits individually or can
> > we combine some of them?
>
> Firstly, what I want to convey is that the structure of each of these
> packets is entirely different. Therefore, in terms of ethtool, since
> packets with the same structure are considered a flow, I understand
> that it is necessary to define such different things (I actually think
> that the people at Intel are doing it that way).
>
> Let me first explain the difference between GTPC and GTPU.
> The UDP ports are different in GTPC and GTPU.
> What's further different is that in the case of GTPC, GTPv2-C is used,
> and in the case of GTPU, GTPv1-U is used, which are mainstream in
> current mobile communications.
>
> Especially the uniqueness of GTPC communication varies according to
> the processing phase.
> CSR (Create Session Request) starts processing from a state where TEID
> is not included. Therefore, it is separated into cases where packets
> have TEID and where they don’t.
> Of course, there are cases where we want to specially process only the
> communication without TEID, and just creating a session is one of the
> more vulnerable parts of the mobile network.
>
> EH stands for Extension Header.
> This is the case with GTPU packets compatible with 5G. If it’s the
> Flow Director, it reads a parameter related to QoS called QFI.
> Without this, it is impossible to process GTPv1 packets compatible with 5G.
> Furthermore, this Extension Header has parts where the shape differs
> depending on UL/DL, which is called the PDU Session Container.
>
> Specific use cases basically apply to services that terminate GTP itself.
>
> The structure of processing in RSS with ethtool until now was to
> select a fixed shape of packets and parameters of those packets to
> perform RSS.
> Conforming to this format is why it becomes so numerous.
>
>
> 2023年10月17日(火) 7:23 Jakub Kicinski <kuba@xxxxxxxxxx>:
>
> >
> > Thanks for the v2!
> >
> > Adding Willem, Pablo, and Harald to CC (please CC them on future
> > versions).
> >
> > On Thu, 12 Oct 2023 06:01:15 +0000 Takeru Hayasaka wrote:
> > > diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> > > index f7fba0dc87e5..a2d4f2081cf3 100644
> > > --- a/include/uapi/linux/ethtool.h
> > > +++ b/include/uapi/linux/ethtool.h
> > > @@ -2011,6 +2011,18 @@ static inline int ethtool_validate_duplex(__u8 duplex)
> > >  #define      IPV4_FLOW       0x10    /* hash only */
> > >  #define      IPV6_FLOW       0x11    /* hash only */
> > >  #define      ETHER_FLOW      0x12    /* spec only (ether_spec) */
> > > +#define GTPU_V4_FLOW 0x13    /* hash only */
> > > +#define GTPU_V6_FLOW 0x14    /* hash only */
> > > +#define GTPC_V4_FLOW 0x15    /* hash only */
> > > +#define GTPC_V6_FLOW 0x16    /* hash only */
> > > +#define GTPC_TEID_V4_FLOW 0x17       /* hash only */
> > > +#define GTPC_TEID_V6_FLOW 0x18       /* hash only */
> > > +#define GTPU_EH_V4_FLOW 0x19 /* hash only */
> > > +#define GTPU_EH_V6_FLOW 0x20 /* hash only */
> >
> > nit: please note that these are hex numbers,
> >      next value after 0x19 is 0x1a, not 0x20.
> >
> > > +#define GTPU_UL_V4_FLOW 0x21 /* hash only */
> > > +#define GTPU_UL_V6_FLOW 0x22 /* hash only */
> > > +#define GTPU_DL_V4_FLOW 0x23 /* hash only */
> > > +#define GTPU_DL_V6_FLOW 0x24 /* hash only */
> > >  /* Flag to enable additional fields in struct ethtool_rx_flow_spec */
> > >  #define      FLOW_EXT        0x80000000
> > >  #define      FLOW_MAC_EXT    0x40000000
> >
> > What gives me pause here is the number of flow sub-types we define
> > for GTP hashing.
> >
> > My understanding of GTP is limited to what I just read on Wikipedia.
> >
> > IIUC the GTPC vs GTPU distinction comes down to the UDP port on
> > which the protocol runs? Are the frames also different?
> >
> > I'm guessing UL/DL are uplink/downlink but what's EH?
> >
> > How do GTPU_V4_FLOW, GTPU_EH_V4_FLOW, GTPU_UL_V4_FLOW, and
> > GTPU_DL_V4_FLOW differ?
> >
> > Key question is - are there reasonable use cases that you can think of
> > for enabling GTP hashing for each one of those bits individually or can
> > we combine some of them?
> >
> > > @@ -2025,6 +2037,7 @@ static inline int ethtool_validate_duplex(__u8 duplex)
> > >  #define      RXH_IP_DST      (1 << 5)
> > >  #define      RXH_L4_B_0_1    (1 << 6) /* src port in case of TCP/UDP/SCTP */
> > >  #define      RXH_L4_B_2_3    (1 << 7) /* dst port in case of TCP/UDP/SCTP */
> > > +#define      RXH_GTP_TEID    (1 << 8) /* teid in case of GTP */
> > >  #define      RXH_DISCARD     (1 << 31)