Re: [PATCH net-next v20 00/12] virtio_net: Add ethtool flow rules support

["Michael S. Tsirkin" <mst@redhat.com>]

On Sat, Feb 07, 2026 at 07:14:25AM -0600, Dan Jurgens wrote:
> On 2/7/26 4:01 AM, Michael S. Tsirkin wrote:
> > On Thu, Feb 05, 2026 at 06:43:28PM -0800, Jakub Kicinski wrote:
> >> On Thu, 5 Feb 2026 16:46:55 -0600 Daniel Jurgens wrote:
> >>> This series implements ethtool flow rules support for virtio_net using the
> >>> virtio flow filter (FF) specification. The implementation allows users to
> >>> configure packet filtering rules through ethtool commands, directing
> >>> packets to specific receive queues, or dropping them based on various
> >>> header fields.
> >>
> >> This is a 4th version of this you posted in as many days and it doesn't
> >> even build. Please slow down. Please wait with v21 until after the merge
> >> window. We have enough patches to sift thru still for v7.0.
> > 
> > v20 and no end in sight.
> > Just looking at the amount of pain all this parsing is inflicting
> > makes me worry. And wait until we need to begin worrying about
> > maintaining UAPI stability.
> > 
> > It would be much nicer if drivers were out of the business of parsing
> > fiddly structures.  Isn't there a way for more code in net core
> > to deal with all this?
> 
> MST, you reviewed the spec that defined these data structures. If you
> didn't want the driver to have parse data structures then suggesting
> using the same format as the ethtool flow specs would have been a great
> idea at that point. Or short of that padded and fixed size data
> structures would also made things much cleaner.

Oh virtio is actually reasonably nice IMHO, and of course
virtio net has to parse them. But a bunch of issues
I reported are around parsing uapi/linux/ethtool.h structures.
There is a ton of code like this:


+static void parse_ip4(struct iphdr *mask, struct iphdr *key,
+                     const struct ethtool_rx_flow_spec *fs)
+{
+       const struct ethtool_usrip4_spec *l3_mask = &fs->m_u.usr_ip4_spec;
+       const struct ethtool_usrip4_spec *l3_val  = &fs->h_u.usr_ip4_spec;
+
+       if (l3_mask->ip4src) {
+               memcpy(&mask->saddr, &l3_mask->ip4src, sizeof(mask->saddr));
+               memcpy(&key->saddr, &l3_val->ip4src, sizeof(key->saddr));
+       }
+
+       if (l3_mask->ip4dst) {
+               memcpy(&mask->daddr, &l3_mask->ip4dst, sizeof(mask->daddr));
+               memcpy(&key->daddr, &l3_val->ip4dst, sizeof(key->daddr));
+       }
+
+       if (l3_mask->tos) {
+               mask->tos = l3_mask->tos;
+               key->tos = l3_val->tos;
+       }
+}
+

and I just ask, given there's apparently nothing at all here
that is driver specific, whether it is generally useful
enough to live in net core?


> I thought this series was close to done, so I was trying to address the
> very non-deterministic AI review comments. It's been generating new
> comments on things that had been there for many revisions, and running
> it locally with the same model never reproduces the comments from the
> online review.

Uncritically posting, or trying to address "ai review" comments is not
at all a good idea.


A bigger problem is that we are still not done finding bugs in this.
My thought therefore was to see if we can move more code to
net core where more *humans* will read it and help find and fix bug.

-- 
MST