RE: [EXT] Re: [v1,ethtool] ethtool: add setting frame preemption of traffic classes
From: Po Liu
Date: Wed Dec 04 2019 - 03:14:42 EST
Hi Murali, Ivan,
Thank you for your feedback. Maybe it is better to use "RFC" type for these patches for the discussion.
Br,
Po Liu
> -----Original Message-----
> From: Murali Karicheri <m-karicheri2@xxxxxx>
> Sent: 2019å12æ4æ 1:42
> To: Po Liu <po.liu@xxxxxxx>; rmk+kernel@xxxxxxxxxxxxxxx;
> linville@xxxxxxxxxxxxx; netdev-owner@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx;
> vinicius.gomes@xxxxxxxxx; simon.horman@xxxxxxxxxxxxx; Claudiu Manoil
> <claudiu.manoil@xxxxxxx>; Vladimir Oltean <vladimir.oltean@xxxxxxx>;
> Xiaoliang Yang <xiaoliang.yang_1@xxxxxxx>; Roy Zang <roy.zang@xxxxxxx>;
> Mingkai Hu <mingkai.hu@xxxxxxx>; Jerry Huang <jerry.huang@xxxxxxx>; Leo
> Li <leoyang.li@xxxxxxx>
> Subject: [EXT] Re: [v1,ethtool] ethtool: add setting frame preemption of traffic
> classes
>
> Caution: EXT Email
>
> Hi Po Liu,
>
> Thanks for working on this! Some suggestion below as we are working on adding
> this support to Texas Instrument's CPSW driver on AM65x family of SoCs as well.
> TRM for that is provided below for your reference.
> Relevant section for IET (Frame pre-emption) is
>
> 12.2.1.4.6.6.1IET Configuration
>
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.ti.c
> om%2Flit%2Fug%2Fspruid7d%2Fspruid7d.pdf&data=02%7C01%7Cpo.liu%
> 40nxp.com%7C15544a9db1ff422e2fdf08d77817647f%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C1%7C637109914194485742&sdata=VD5kg%
> 2FY1SDDWjwMZHjgUNlFrcvXnDvdIPGblWkx4DXs%3D&reserved=0
>
> On 12/03/2019 11:27 AM, Ivan Khoronzhuk wrote:
> > On Wed, Nov 27, 2019 at 09:58:52AM +0000, Po Liu wrote:
> >
> > Hi Po Liu,
> >
> >> IEEE Std 802.1Qbu standard defined the frame preemption of port
> >> trffic classes. User can set a value to hardware. The value will be
> >> translated to a binary, each bit represent a traffic class.
> >> Bit "1" means preemptable traffic class. Bit "0" means express
> >> traffic class. MSB represent high number traffic class.
> >>
> >> ethtool -k devname
> >>
> >> This command would show if the tx-preemption feature is available.
> >> If hareware set preemption feature. The property would be a fixed
> >> value 'on' if hardware support the frame preemption. Feature would
> >> show a fixed value 'off' if hardware don't support the frame preemption.
> >>
> >> ethtool devname
> >>
> >> This command would show include an item 'preemption'. A following
> >> value '0' means all traffic classes are 'express'. A value none zero
> >> means traffic classes preemption capabilities. The value will be
> >> translated to a binary, each bit represent a traffic class. Bit '1'
> >> means preemptable traffic class. Bit '0' means express traffic class.
> >> MSB represent high number traffic class.
> >>
> >> ethtool -s devname preemption N
> >
> > What about other potential parameters like MAC fragment size, mac hold?
> > Shouldn't be it considered along with other FP parameters to provide
> > correct interface later?
> >
> > Say, preemption, lets name it fp-mask or frame-preemption-mask.
> > Then other potential setting can be similar and added later:
> >
> > frame-preemption-mask
> > frame-preemption-fragsize
> > frame-preemption-machold
> Need additional capabilities as described by Ivan above. Thanks Ivan!
>
> So it would be better to use feature/sub-parameter format so that it can be
> extended as needed in the future.
>
> For express/Preemptable mask setting it becomes
>
> ethtool -s devname frame-preemption tc-mask N
>
> For setting min fragment size
>
> ethtool -s devname frame-preemption min-fragsize 64
>
I thought the fragment size set 64 is enough for Qbv. Anyway, if you prefer more details setting. I think it is better to set a specific serial type. For example, '-p' for set tc-mask N and min-fragsize 64, '-P' for status.
> Also the device may be capable of doing Verify process to detect the capability
> of neighbor device and show the status. So we should have a way to show this
The verify process maybe disabled as default.
> status as well when user type
>
> ethtool devname
>
> We are working currently to add this feature to CPSW driver on AM65x which
> will be upstream-ed soon. So want to have this done in an way that we can
> extend it later.
> Similarly for taprio, there are some parameters that user might want to tune
> such as Max SDU size per tc class. I hope we could use ethtool to set the same
> on a similar way.
Furthermore, this setting could be extend for a serial setting for mac and traffic class.
>
> Thanks
>
> Murali
>
> > ....
> >
> > mac-hold it's rather flag, at least I've used it as priv-flag.
> > so can or so
> >
> > frame-preemption-flags
> >
> >>
> >> This command would set which traffic classes are frame preemptable.
> >> The value will be translated to a binary, each bit represent a
> >> traffic class. Bit '1' means preemptable traffic class. Bit '0'
> >> means express traffic class. MSB represent high number traffic class.
> >>
> >> Signed-off-by: Po Liu <Po.Liu@xxxxxxx>
> >> ---
> >> ethtool-copy.h |Â 6 +++++-
> >> ethtool.8.in |Â 8 ++++++++
> >> ethtool.c | 18 ++++++++++++++++++
> >> 3 files changed, 31 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/ethtool-copy.h b/ethtool-copy.h index 9afd2e6..e04bdf3
> >> 100644
> >> --- a/ethtool-copy.h
> >> +++ b/ethtool-copy.h
> >> @@ -1662,6 +1662,9 @@ static __inline__ int
> >> ethtool_validate_duplex(__u8 duplex)
> >> #define AUTONEG_DISABLE 0x00
> >> #define AUTONEG_ENABLE 0x01
> >>
> >> +/* Disable preemtion. */
> >> +#define PREEMPTION_DISABLE 0x0
> >> +
> >> /* MDI or MDI-X status/control - if MDI/MDI_X/AUTO is set then
> >> * the driver is required to renegotiate link */ @@ -1878,7 +1881,8
> >> @@ struct ethtool_link_settings {
> >> __s8 link_mode_masks_nwords;
> >> __u8 transceiver;
> >> __u8 reserved1[3];
> >> - __u32 reserved[7];
> >> + __u32 preemption;
> >> + __u32 reserved[6];
> >> __u32 link_mode_masks[0];
> >> /* layout of link_mode_masks fields:
> >> * __u32 map_supported[link_mode_masks_nwords];
> >> diff --git a/ethtool.8.in b/ethtool.8.in index 062695a..7d612b2
> >> 100644
> >> --- a/ethtool.8.in
> >> +++ b/ethtool.8.in
> >> @@ -236,6 +236,7 @@ ethtool \- query or control network driver and
> >> hardware settings
> >> .B2 autoneg on off
> >> .BN advertise
> >> .BN phyad
> >> +.BN preemption
> >> .B2 xcvr internal external
> >> .RB [ wol \ \*(WO]
> >> .RB [ sopass \ \*(MA]
> >> @@ -703,6 +704,13 @@ lB l lB.
> >> .BI phyad \ N
> >> PHY address.
> >> .TP
> >> +.BI preemption \ N
> >> +Set preemptable traffic classes by bits.
> >> +.B A
> >> +value will be translated to a binary, each bit represent a traffic
> >> class.
> >> +Bit "1" means preemptable traffic class. Bit "0" means express
> >> traffic class.
> >> +MSB represent high number traffic class.
> >> +.TP
> >> .A2 xcvr internal external
> >> Selects transceiver type. Currently only internal and external can be
> >> specified, in the future further types might be added.
> >> diff --git a/ethtool.c b/ethtool.c
> >> index acf183d..d5240f8 100644
> >> --- a/ethtool.c
> >> +++ b/ethtool.c
> >> @@ -928,6 +928,12 @@ dump_link_usettings(const struct
> >> ethtool_link_usettings *link_usettings)
> >> }
> >> }
> >>
> >> + if (link_usettings->base.preemption == PREEMPTION_DISABLE)
> >> + fprintf(stdout, "ÂÂÂ Preemption: 0x0 (off)\n");
> >> + else
> >> + fprintf(stdout, "ÂÂÂ Preemption: 0x%x\n",
> >> + link_usettings->base.preemption);
> >> +
> >> return 0;
> >> }
> >>
> >> @@ -2869,6 +2875,7 @@ static int do_sset(struct cmd_context *ctx)
> >> int port_wanted = -1;
> >> int mdix_wanted = -1;
> >> int autoneg_wanted = -1;
> >> + int preemption_wanted = -1;
> >> int phyad_wanted = -1;
> >> int xcvr_wanted = -1;
> >> u32 *full_advertising_wanted = NULL; @@ -2957,6 +2964,12 @@
> >> static int do_sset(struct cmd_context *ctx)
> >> } else {
> >> exit_bad_args();
> >> }
> >> + } else if (!strcmp(argp[i], "preemption")) {
> >> + gset_changed = 1;
> >> + i += 1;
> >> + if (i >= argc)
> >> + exit_bad_args();
> >> + preemption_wanted = get_u32(argp[i], 16);
> >> } else if (!strcmp(argp[i], "advertise")) {
> >> gset_changed = 1;
> >> i += 1;
> >> @@ -3094,6 +3107,9 @@ static int do_sset(struct cmd_context *ctx)
> >> }
> >> if (autoneg_wanted != -1)
> >> link_usettings->base.autoneg = autoneg_wanted;
> >> + if (preemption_wanted != -1)
> >> + link_usettings->base.preemption
> >> + = preemption_wanted;
> >> if (phyad_wanted != -1)
> >> link_usettings->base.phy_address = phyad_wanted;
> >> if (xcvr_wanted != -1)
> >> @@ -3186,6 +3202,8 @@ static int do_sset(struct cmd_context *ctx)
> >> fprintf(stderr, "Â not setting transceiver\n");
> >> if (mdix_wanted != -1)
> >> fprintf(stderr, "Â not setting mdix\n");
> >> + if (preemption_wanted != -1)
> >> + fprintf(stderr, "Â not setting preemption\n");
> >> }
> >> }
> >>
> >> --
> >> 2.17.1
> >>
> >