Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 23 Jan 2021 18:03:48 +0200
From:   Ido Schimmel <idosch@idosch.org>
To:     Oleksandr Mazur <oleksandr.mazur@plvision.eu>
Cc:     Jakub Kicinski <kuba@kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "jiri@nvidia.com" <jiri@nvidia.com>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net-next] net: core: devlink: add new trap action
 HARD_DROP
Message-ID: <20210123160348.GB2799851@shredder.lan>
References: <AM0P190MB073828252FFDA3215387765CE4A00@AM0P190MB0738.EURP190.PROD.OUTLOOK.COM>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <AM0P190MB073828252FFDA3215387765CE4A00@AM0P190MB0738.EURP190.PROD.OUTLOOK.COM>
Precedence: bulk

On Fri, Jan 22, 2021 at 08:36:01AM +0000, Oleksandr Mazur wrote:
> On Thu, 21 Jan 2021 14:21:52 +0200 Ido Schimmel wrote:
> > On Thu, Jan 21, 2021 at 01:29:37PM +0200, Oleksandr Mazur wrote:
> > > Add new trap action HARD_DROP, which can be used by the
> > > drivers to register traps, where it's impossible to get
> > > packet reported to the devlink subsystem by the device
> > > driver, because it's impossible to retrieve dropped packet
> > > from the device itself.
> > > In order to use this action, driver must also register
> > > additional devlink operation - callback that is used
> > > to retrieve number of packets that have been dropped by
> > > the device.? 
> > 
> > Are these global statistics about number of packets the hardware dropped
> > for a specific reason or are these per-port statistics?
> > 
> > It's a creative use of devlink-trap interface, but I think it makes
> > sense. Better to re-use an existing interface than creating yet another
> > one.
> 
> > Not sure if I agree, if we can't trap why is it a trap?
> > It's just a counter.
> 
> It's just another ACTION for trap item. Action however can be switched, e.g. from HARD_DROP to MIRROR.
> 
> The thing is to be able to configure specific trap to be dropped, and provide a way for the device to report back how many packets have been dropped.
> If device is able to report the packet itself, then devlink would be in charge of counting. If not, there should be a way to retrieve these statistics from the devlink.

So no need for another action. Just report these stats via
'DEVLINK_ATTR_STATS_RX_DROPPED' if the hardware supports it.

Currently you do:

+static int
+devlink_trap_hard_drop_stats_put(struct sk_buff *msg,
+				 struct devlink *devlink,
+				 const struct devlink_trap_item *trap_item)
+{
+	struct nlattr *attr;
+	u64 drops;
+	int err;
+
+	err = devlink->ops->trap_hard_drop_counter_get(devlink, trap_item->trap,
+						       &drops);
+	if (err)
+		return err;
+
+	attr = nla_nest_start(msg, DEVLINK_ATTR_STATS);
+	if (!attr)
+		return -EMSGSIZE;
+
+	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_STATS_RX_DROPPED, drops,
+			      DEVLINK_ATTR_PAD))
+		goto nla_put_failure;
+
+	nla_nest_end(msg, attr);
+
+	return 0;
+
+nla_put_failure:
+	nla_nest_cancel(msg, attr);
+	return -EMSGSIZE;
+}
+
 static int devlink_nl_trap_fill(struct sk_buff *msg, struct devlink *devlink,
 				const struct devlink_trap_item *trap_item,
 				enum devlink_command cmd, u32 portid, u32 seq,
@@ -6857,7 +6889,10 @@ static int devlink_nl_trap_fill(struct sk_buff *msg, struct devlink *devlink,
 	if (err)
 		goto nla_put_failure;
 
-	err = devlink_trap_stats_put(msg, trap_item->stats);
+	if (trap_item->action == DEVLINK_TRAP_ACTION_HARD_DROP)
+		err = devlink_trap_hard_drop_stats_put(msg, devlink, trap_item);
+	else
+		err = devlink_trap_stats_put(msg, trap_item->stats);
 	if (err)
 		goto nla_put_failure;

Which means that user space will see stats come and go based on the
trap's action. That's not desirable. Instead, change:

[DEVLINK_ATTR_STATS]
	[DEVLINK_ATTR_STATS_RX_PACKETS]
	[DEVLINK_ATTR_STATS_RX_BYTES]

To:

[DEVLINK_ATTR_STATS]
	[DEVLINK_ATTR_STATS_RX_PACKETS]
	[DEVLINK_ATTR_STATS_RX_BYTES]
	[DEVLINK_ATTR_STATS_RX_DROPPED]

Where the last attribute is reported to user space for devices that
support such stats. No changes required in uAPI / iproute2.