[v2,net-next,7/9] net: netdevsim: mimic tc-taprio offload

Commit Message

Vladimir Oltean June 13, 2023, 9:54 p.m. UTC

To be able to use netdevsim for tc-testing with an offloaded tc-taprio
schedule, it needs to report a PTP clock (which it now does), and to
accept ndo_setup_tc(TC_SETUP_QDISC_TAPRIO) calls.

Since netdevsim has no packet I/O, this doesn't do anything intelligent,
it only allows taprio offload code paths to go through some level of
automated testing.

Signed-off-by: Vladimir Oltean <[e-mail]>
---
v1->v2: patch is new

 drivers/net/netdevsim/netdev.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

Comments

Vinicius Costa Gomes June 15, 2023, 12:06 a.m. UTC | #1

Vladimir Oltean <[e-mail]> writes:

> To be able to use netdevsim for tc-testing with an offloaded tc-taprio
> schedule, it needs to report a PTP clock (which it now does), and to
> accept ndo_setup_tc(TC_SETUP_QDISC_TAPRIO) calls.
>
> Since netdevsim has no packet I/O, this doesn't do anything intelligent,
> it only allows taprio offload code paths to go through some level of
> automated testing.
>
> Signed-off-by: Vladimir Oltean <[e-mail]>
> ---
> v1->v2: patch is new
>
>  drivers/net/netdevsim/netdev.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index 58cd51de5b79..e26be4bd0d90 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -209,6 +209,31 @@ static int nsim_set_vf_link_state(struct net_device *dev, int vf, int state)
>  	return 0;
>  }
>  
> +static void nsim_taprio_stats(struct tc_taprio_qopt_stats *stats)
> +{
> +	stats->window_drops = 0;
> +	stats->tx_overruns = 0;
> +}
> +
> +static int nsim_setup_tc_taprio(struct net_device *dev,
> +				struct tc_taprio_qopt_offload *offload)
> +{
> +	int err = 0;
> +
> +	switch (offload->cmd) {
> +	case TAPRIO_CMD_REPLACE:
> +	case TAPRIO_CMD_DESTROY:
> +		break;

I was thinking about how useful would proper validation of the
parameters be? Thinking that we could detect "driver API" breakages
earlier, and we want it documented that the drivers should check for the
things that it supports.

Makes sense?

> +	case TAPRIO_CMD_STATS:
> +		nsim_taprio_stats(&offload->stats);
> +		break;
> +	default:
> +		err = -EOPNOTSUPP;
> +	}
> +
> +	return err;
> +}
> +
>  static LIST_HEAD(nsim_block_cb_list);
>  
>  static int
> @@ -217,6 +242,8 @@ nsim_setup_tc(struct net_device *dev, enum tc_setup_type type, void *type_data)
>  	struct netdevsim *ns = netdev_priv(dev);
>  
>  	switch (type) {
> +	case TC_SETUP_QDISC_TAPRIO:
> +		return nsim_setup_tc_taprio(dev, type_data);
>  	case TC_SETUP_BLOCK:
>  		return flow_block_cb_setup_simple(type_data,
>  						  &nsim_block_cb_list,
> -- 
> 2.34.1
>

Cheers,

Vladimir Oltean Aug. 1, 2023, 4:45 p.m. UTC | #2

On Wed, Jun 14, 2023 at 05:06:24PM -0700, Vinicius Costa Gomes wrote:
> > +static int nsim_setup_tc_taprio(struct net_device *dev,
> > +				struct tc_taprio_qopt_offload *offload)
> > +{
> > +	int err = 0;
> > +
> > +	switch (offload->cmd) {
> > +	case TAPRIO_CMD_REPLACE:
> > +	case TAPRIO_CMD_DESTROY:
> > +		break;
> 
> I was thinking about how useful would proper validation of the
> parameters be? Thinking that we could detect "driver API" breakages
> earlier, and we want it documented that the drivers should check for the
> things that it supports.
> 
> Makes sense?

Sorry, I lack imagination as to what the netdevsim driver may check for.
The taprio offload parameters should always be valid, properly speaking,
otherwise the Qdisc wouldn't be passing them on to the driver. At least
that would be the intention. The rest are hardware specific checks for
hardware specific limitations. Here there is no hardware.

The parameters passed to TAPRIO_CMD_REPLACE are:

struct tc_mqprio_qopt_offload mqprio:
	struct tc_mqprio_qopt qopt: validated by taprio_parse_mqprio_opt() for flags 0x2
	u16 mode: always set to TC_MQPRIO_MODE_DCB
	u16 shaper: always set to TC_MQPRIO_SHAPER_DCB
	u32 flags: always set to 0
	u64 min_rate[TC_QOPT_MAX_QUEUE]: always set to [0,]
	u64 max_rate[TC_QOPT_MAX_QUEUE]: always set to [0,]
	unsigned long preemptible_tcs: always set to 0, because ethtool_dev_mm_supported() returns false

ktime_t base_time: any value is valid

u64 cycle_time: any value is valid

u64 cycle_time_extension: any value <= cycle_time is valid. According to 802.1Q
			  "Q.5 CycleTimeExtension variables", it's the maximum
			  amount by which the penultimate cycle can be extended
			  to avoid a very short cycle upon a ConfigChange event.
			  But if CycleTimeExtension is larger than one CycleTime,
			  then we're not even talking about the penultimate cycle
			  anymore, but about ones previous to that?! Maybe this
			  should be limited to 0 <= cycle_time_extension <= cycle_time
			  by taprio, certainly not by offloading drivers.

u32 max_sdu[TC_MAX_QUEUE]: limited to a value <= dev->max_mtu by taprio

size_t num_entries: any value is valid

struct tc_taprio_sched_entry entries[]:
	u8 command: will be either one of: TC_TAPRIO_CMD_SET_GATES, TC_TAPRIO_CMD_SET_AND_HOLD
		    or TC_TAPRIO_CMD_SET_AND_RELEASE. However 802.1Q "Table 8-7—Gate operations"
		    says "If frame preemption is not supported or not enabled (preemptionActive is
		    FALSE), this operation behaves the same as SetGateStates.". So I
		    see no reason to enforce any restriction here either?

	u32 gate_mask: technically can have bits set, which correspond
		       to traffic classes larger than dev->num_tc.
		       Taprio can enforce this, so I wouldn't see
		       drivers beginning to feel paranoid about it.
		       Actually I had a patch about this:
		       https://patchwork.kernel.org/project/netdevbpf/patch/[e-mail]/
		       but I decided to drop it because I didn't have
		       any strong case for it.
	u32 interval: any value is valid. If the sum of entry intervals
		      is less than the cycle_time, again that's taprio's
		      problem to check for, in its netlink attribute
		      validation method rather than offloading drivers.

There are no parameters given to TAPRIO_CMD_DESTROY.

Vinicius Costa Gomes Aug. 1, 2023, 5:39 p.m. UTC | #3

Hi Vladimir,

Vladimir Oltean <[e-mail]> writes:

> On Wed, Jun 14, 2023 at 05:06:24PM -0700, Vinicius Costa Gomes wrote:
>> > +static int nsim_setup_tc_taprio(struct net_device *dev,
>> > +				struct tc_taprio_qopt_offload *offload)
>> > +{
>> > +	int err = 0;
>> > +
>> > +	switch (offload->cmd) {
>> > +	case TAPRIO_CMD_REPLACE:
>> > +	case TAPRIO_CMD_DESTROY:
>> > +		break;
>> 
>> I was thinking about how useful would proper validation of the
>> parameters be? Thinking that we could detect "driver API" breakages
>> earlier, and we want it documented that the drivers should check for the
>> things that it supports.
>> 
>> Makes sense?
>
> Sorry, I lack imagination as to what the netdevsim driver may check for.
> The taprio offload parameters should always be valid, properly speaking,
> otherwise the Qdisc wouldn't be passing them on to the driver. At least
> that would be the intention. The rest are hardware specific checks for
> hardware specific limitations. Here there is no hardware.
>

Trying to remember what was going through my mind when I said that.

What I seem to recall is something that would help us "keep honest":
I was worrying about someone (perhaps myself ;-) sneaking a new feature
in taprio and forgetting to update other drivers.

I thought that adding a check for the existing parameters would help
detect those kind of things. If anything unknown was there in the
offload struct, netdevsim would complain loudly.

Perhaps I was worrying too much. And the way to solve that is to keep
active attention against that during review.

> The parameters passed to TAPRIO_CMD_REPLACE are:
>
> struct tc_mqprio_qopt_offload mqprio:
> 	struct tc_mqprio_qopt qopt: validated by taprio_parse_mqprio_opt() for flags 0x2
> 	u16 mode: always set to TC_MQPRIO_MODE_DCB
> 	u16 shaper: always set to TC_MQPRIO_SHAPER_DCB
> 	u32 flags: always set to 0
> 	u64 min_rate[TC_QOPT_MAX_QUEUE]: always set to [0,]
> 	u64 max_rate[TC_QOPT_MAX_QUEUE]: always set to [0,]
> 	unsigned long preemptible_tcs: always set to 0, because ethtool_dev_mm_supported() returns false
>
> ktime_t base_time: any value is valid
>
> u64 cycle_time: any value is valid
>
> u64 cycle_time_extension: any value <= cycle_time is valid. According to 802.1Q
> 			  "Q.5 CycleTimeExtension variables", it's the maximum
> 			  amount by which the penultimate cycle can be extended
> 			  to avoid a very short cycle upon a ConfigChange event.
> 			  But if CycleTimeExtension is larger than one CycleTime,
> 			  then we're not even talking about the penultimate cycle
> 			  anymore, but about ones previous to that?! Maybe this
> 			  should be limited to 0 <= cycle_time_extension <= cycle_time
> 			  by taprio, certainly not by offloading drivers.
>

Good point. I have to review 802.1Q, but from what I remember that
sounds right, cycle_time_extension greater than cycle_time doesn't make
much sense. Having a check for it in taprio itself sounds good.

> u32 max_sdu[TC_MAX_QUEUE]: limited to a value <= dev->max_mtu by taprio
>
> size_t num_entries: any value is valid
>
> struct tc_taprio_sched_entry entries[]:
> 	u8 command: will be either one of: TC_TAPRIO_CMD_SET_GATES, TC_TAPRIO_CMD_SET_AND_HOLD
> 		    or TC_TAPRIO_CMD_SET_AND_RELEASE. However 802.1Q "Table 8-7—Gate operations"
> 		    says "If frame preemption is not supported or not enabled (preemptionActive is
> 		    FALSE), this operation behaves the same as SetGateStates.". So I
> 		    see no reason to enforce any restriction here either?
>
> 	u32 gate_mask: technically can have bits set, which correspond
> 		       to traffic classes larger than dev->num_tc.
> 		       Taprio can enforce this, so I wouldn't see
> 		       drivers beginning to feel paranoid about it.
> 		       Actually I had a patch about this:
> 		       https://patchwork.kernel.org/project/netdevbpf/patch/[e-mail]/
> 		       but I decided to drop it because I didn't have
> 		       any strong case for it.
> 	u32 interval: any value is valid. If the sum of entry intervals
> 		      is less than the cycle_time, again that's taprio's
> 		      problem to check for, in its netlink attribute
> 		      validation method rather than offloading drivers.
>

Thank you for the time it took to give this amount of detail.


Cheers,

Vladimir Oltean Aug. 1, 2023, 5:43 p.m. UTC | #4

On Tue, Aug 01, 2023 at 10:39:23AM -0700, Vinicius Costa Gomes wrote:
> Hi Vladimir,
> 
> Vladimir Oltean <[e-mail]> writes:
> 
> > On Wed, Jun 14, 2023 at 05:06:24PM -0700, Vinicius Costa Gomes wrote:
> >> > +static int nsim_setup_tc_taprio(struct net_device *dev,
> >> > +				struct tc_taprio_qopt_offload *offload)
> >> > +{
> >> > +	int err = 0;
> >> > +
> >> > +	switch (offload->cmd) {
> >> > +	case TAPRIO_CMD_REPLACE:
> >> > +	case TAPRIO_CMD_DESTROY:
> >> > +		break;
> >> 
> >> I was thinking about how useful would proper validation of the
> >> parameters be? Thinking that we could detect "driver API" breakages
> >> earlier, and we want it documented that the drivers should check for the
> >> things that it supports.
> >> 
> >> Makes sense?
> >
> > Sorry, I lack imagination as to what the netdevsim driver may check for.
> > The taprio offload parameters should always be valid, properly speaking,
> > otherwise the Qdisc wouldn't be passing them on to the driver. At least
> > that would be the intention. The rest are hardware specific checks for
> > hardware specific limitations. Here there is no hardware.
> >
> 
> Trying to remember what was going through my mind when I said that.
> 
> What I seem to recall is something that would help us "keep honest":
> I was worrying about someone (perhaps myself ;-) sneaking a new feature
> in taprio and forgetting to update other drivers.
> 
> I thought that adding a check for the existing parameters would help
> detect those kind of things. If anything unknown was there in the
> offload struct, netdevsim would complain loudly.
> 
> Perhaps I was worrying too much. And the way to solve that is to keep
> active attention against that during review.

Ok, so I'm not making any change to the patch set as a result of this
comment, would you agree?

Vinicius Costa Gomes Aug. 1, 2023, 6:06 p.m. UTC | #5

Vladimir Oltean <[e-mail]> writes:

> On Tue, Aug 01, 2023 at 10:39:23AM -0700, Vinicius Costa Gomes wrote:
>> Hi Vladimir,
>> 
>> Vladimir Oltean <[e-mail]> writes:
>> 
>> > On Wed, Jun 14, 2023 at 05:06:24PM -0700, Vinicius Costa Gomes wrote:
>> >> > +static int nsim_setup_tc_taprio(struct net_device *dev,
>> >> > +				struct tc_taprio_qopt_offload *offload)
>> >> > +{
>> >> > +	int err = 0;
>> >> > +
>> >> > +	switch (offload->cmd) {
>> >> > +	case TAPRIO_CMD_REPLACE:
>> >> > +	case TAPRIO_CMD_DESTROY:
>> >> > +		break;
>> >> 
>> >> I was thinking about how useful would proper validation of the
>> >> parameters be? Thinking that we could detect "driver API" breakages
>> >> earlier, and we want it documented that the drivers should check for the
>> >> things that it supports.
>> >> 
>> >> Makes sense?
>> >
>> > Sorry, I lack imagination as to what the netdevsim driver may check for.
>> > The taprio offload parameters should always be valid, properly speaking,
>> > otherwise the Qdisc wouldn't be passing them on to the driver. At least
>> > that would be the intention. The rest are hardware specific checks for
>> > hardware specific limitations. Here there is no hardware.
>> >
>> 
>> Trying to remember what was going through my mind when I said that.
>> 
>> What I seem to recall is something that would help us "keep honest":
>> I was worrying about someone (perhaps myself ;-) sneaking a new feature
>> in taprio and forgetting to update other drivers.
>> 
>> I thought that adding a check for the existing parameters would help
>> detect those kind of things. If anything unknown was there in the
>> offload struct, netdevsim would complain loudly.
>> 
>> Perhaps I was worrying too much. And the way to solve that is to keep
>> active attention against that during review.
>
> Ok, so I'm not making any change to the patch set as a result of this
> comment, would you agree?

Agreed.


Cheers,

Patch

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 58cd51de5b79..e26be4bd0d90 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -209,6 +209,31 @@  static int nsim_set_vf_link_state(struct net_device *dev, int vf, int state)
 	return 0;
 }
 
+static void nsim_taprio_stats(struct tc_taprio_qopt_stats *stats)
+{
+	stats->window_drops = 0;
+	stats->tx_overruns = 0;
+}
+
+static int nsim_setup_tc_taprio(struct net_device *dev,
+				struct tc_taprio_qopt_offload *offload)
+{
+	int err = 0;
+
+	switch (offload->cmd) {
+	case TAPRIO_CMD_REPLACE:
+	case TAPRIO_CMD_DESTROY:
+		break;
+	case TAPRIO_CMD_STATS:
+		nsim_taprio_stats(&offload->stats);
+		break;
+	default:
+		err = -EOPNOTSUPP;
+	}
+
+	return err;
+}
+
 static LIST_HEAD(nsim_block_cb_list);
 
 static int
@@ -217,6 +242,8 @@  nsim_setup_tc(struct net_device *dev, enum tc_setup_type type, void *type_data)
 	struct netdevsim *ns = netdev_priv(dev);
 
 	switch (type) {
+	case TC_SETUP_QDISC_TAPRIO:
+		return nsim_setup_tc_taprio(dev, type_data);
 	case TC_SETUP_BLOCK:
 		return flow_block_cb_setup_simple(type_data,
 						  &nsim_block_cb_list,