Re: [PATCH v7 01/23] net: Introduce direct data placement tcp offload

From: Jakub Kicinski <kuba@kernel.org>
To: Shai Malin <smalin@nvidia.com>
Cc: Aurelien Aptel <aaptel@nvidia.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"edumazet@google.com" <edumazet@google.com>,
	"pabeni@redhat.com" <pabeni@redhat.com>,
	Saeed Mahameed <saeedm@nvidia.com>,
	Tariq Toukan <tariqt@nvidia.com>,
	"leon@kernel.org" <leon@kernel.org>,
	"linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
	"sagi@grimberg.me" <sagi@grimberg.me>, "hch@lst.de" <hch@lst.de>,
	"kbusch@kernel.org" <kbusch@kernel.org>,
	"axboe@fb.com" <axboe@fb.com>,
	Chaitanya Kulkarni <chaitanyak@nvidia.com>,
	Or Gerlitz <ogerlitz@nvidia.com>, Yoray Zack <yorayz@nvidia.com>,
	Boris Pismenny <borisp@nvidia.com>,
	"aurelien.aptel@gmail.com" <aurelien.aptel@gmail.com>,
	"malin1024@gmail.com" <malin1024@gmail.com>
Subject: Re: [PATCH v7 01/23] net: Introduce direct data placement tcp offload
Date: Fri, 28 Oct 2022 08:40:01 -0700	[thread overview]
Message-ID: <20221028084001.447a7c05@kernel.org> (raw)
In-Reply-To: <DM6PR12MB356448156B75DD719E24E41DBC329@DM6PR12MB3564.namprd12.prod.outlook.com>

On Fri, 28 Oct 2022 10:32:22 +0000 Shai Malin wrote:
> > It's a big enough feature to add a genetlink or at least a ethtool
> > command to control. If you add more L5 protos presumably you'll want
> > to control disable / enable separately for them. Also it'd be cleaner
> > to expose the full capabilities and report stats via a dedicated API.
> > Feature bits are not a good fix for complex control-pathy features.  
> 
> With our existing design, we are supporting a bundle of DDP + CRC offload.
> We don't see the value of letting the user control it individually.

I was talking about the L5 parsing you do. I presume it won't be a huge
challenge for you to implement support for framing different than NVMe,
and perhaps even NVMe may have new revisions or things you don't
support? At which point we're gonna have a bit for each protocol? :S
Then there are stats.

We should have a more expressive API here from the get go. TLS offload
is clearly lacking in this area.

> The capabilities bits were added in order to allow future devices which 
> supported only one of the capabilities to plug into the infrastructure 
> or to allow additional capabilities/protocols.
> 
> We could expose the caps via ethtool as regular netdev features, it would 
> make everything simpler and cleaner, but the problem is that features have 
> run out of bits (all 64 are taken, and we understand the challenge with 
> changing that).

Feature bits should be exclusively for information which needs to be
accessed on the fast path, on per packet basis. If you have such a need
then I'm not really opposed to you allocating bits as well, but primary
feature discovery *for the user* should not be over the feature bits.

> We could add a new ethtool command, but on the kernel side it would be 
> quite redundant as we would essentially re-implement feature flag processing 
> (comparing string of features names and enabling bits).
> 
> What do you think?