Re: [PATCH 5.10 462/717] ice, xsk: clear the status bits for the next_to_use descriptor

[Jakub Kicinski]

On Mon, 28 Dec 2020 18:43:36 -0500 Sasha Levin wrote:
> On Mon, Dec 28, 2020 at 02:51:05PM -0800, Jakub Kicinski wrote:
> >On Mon, 28 Dec 2020 17:29:07 -0500 Sasha Levin wrote:  
> >> On Mon, Dec 28, 2020 at 10:54:23AM -0800, Jakub Kicinski wrote:  
> >> >On Mon, 28 Dec 2020 13:47:40 +0100 Greg Kroah-Hartman wrote:  
> >> >> From: Björn Töpel <bjorn.topel@xxxxxxxxx>
> >> >>
> >> >> [ Upstream commit 8d14768a7972b92c73259f0c9c45b969d85e3a60 ]
> >> >>
> >> >> On the Rx side, the next_to_use index points to the next item in the
> >> >> HW ring to be refilled/allocated, and next_to_clean points to the next
> >> >> item to potentially be processed.
> >> >>
> >> >> When the HW Rx ring is fully refilled, i.e. no packets has been
> >> >> processed, the next_to_use will be next_to_clean - 1. When the ring is
> >> >> fully processed next_to_clean will be equal to next_to_use. The latter
> >> >> case is where a bug is triggered.
> >> >>
> >> >> If the next_to_use bits are not cleared, and the "fully processed"
> >> >> state is entered, a stale descriptor can be processed.
> >> >>
> >> >> The skb-path correctly clear the status bit for the next_to_use
> >> >> descriptor, but the AF_XDP zero-copy path did not do that.
> >> >>
> >> >> This change adds the status bits clearing of the next_to_use
> >> >> descriptor.
> >> >>
> >> >> Fixes: 2d4238f55697 ("ice: Add support for AF_XDP")
> >> >> Signed-off-by: Björn Töpel <bjorn.topel@xxxxxxxxx>
> >> >> Signed-off-by: Jakub Kicinski <kuba@xxxxxxxxxx>
> >> >> Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>  
> >> >
> >> >Oh wow, so much for Sasha waiting longer for code to get tested before
> >> >auto-pulling things into stable :/  
> >>
> >> The timeline is usually for a commit to appear in a release, and it did.
> >> Was it too early?  
> >
> >Hm, I'm not sure of exact semantics but I meant a final release,
> >not an -rc.
> >
> >Plus I thought the point of things being part of a release is that
> >people actually get a chance to test that release. -rc1 was cut 24
> >hours ago. I guess a "release" is used as a yardstick here, to
> >measure time, not for practical reasons?  
> 
> Note that it wasn't actually released yet, at this point folks are
> supposed to be testing 5.10.4-rc1 to make sure that those patches are
> okay.
> 
> I still think that there are no significant users of Linus's tree, so
> the idea of having a patch "in a release" doesn't mean as much as folks
> think it does. Sure, we have a lot of folks who test -rc releases, but
> are you aware of anyone who runs -rc on real world workloads to test it?

Ack, I'm not disagreeing. From my limited information/experience testing
seems to happen mostly in the -next trees (be it linux-next or more
likely net-next for networking) and in production releases (either final
produced by Linus or when patch hits some stable tree).

I'd actually be on board of removing this "must be in Linus's tag"
requirement. I don't see why anyone would test rcX instead of just
pulling latest linux/master.

> >> >I have this change and other changes here queued, but haven't sent the
> >> >submission yet.  
> >>
> >> What do you mean with "queued"? Its in Linus's tree for about two weeks
> >> now.  
> >
> >Networking maintainers have their own queue for patches that will go to
> >stable:
> >
> >https://patchwork.kernel.org/bundle/netdev/stable/?state=*  
> 
> This part has always been tricky to me: some parts of net/ and
> drivers/net/ don't go through netdev, and some do. I have a filter to
> ignore net/ completely, but I found that quite a lot of drivers/net/
> wasn't covered by this process.
> 
> How could I blacklist/ignore the parts of the tree you're looking at?
> 
> Also, is drivers/net/ stuff covered as well as net/? I found in the past
> that it's not the case when I was looking at missing patches for the
> hyper-v driver.

Yeah, it's tricky, I've produced some criteria for build testing things
which hit the mailing list:

https://github.com/kuba-moo/nipa/blob/master/netdev/tree_match.py#L47

But it's far from 100% accurate. I think your best shot would be to
filter by committer? :S The special rules only apply to patches which
went directly into net or net-next so committed by DaveM (or myself).