Message-ID: <ad50a349-11be-36bc-fed4-94f5aab3eabd@intel.com>
Date: Tue, 12 Sep 2023 16:41:50 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Paolo Abeni <pabeni@...hat.com>
CC: Andrii Staikov <andrii.staikov@...el.com>,
	<intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>
Subject: Re: [PATCH iwl-next v2] ice: Add support for packet mirroring using
 hardware in switchdev mode

From: Paolo Abeni <pabeni@...hat.com>
Date: Tue, 12 Sep 2023 11:56:15 +0200

> Hi all,
> 
> On Tue, 2023-09-12 at 11:29 +0200, Andrii Staikov wrote:
>> Switchdev mode allows to add mirroring rules to mirror
>> incoming and outgoing packets to the interface's port
>> representor. Previously, this was available only using
>> software functionality. Add possibility to offload this
>> functionality to the NIC hardware.
>>
>> Introduce ICE_MIRROR_PACKET filter action to the
>> ice_sw_fwd_act_type enum to identify the desired action
>> and pass it to the hardware as well as the VSI to mirror.
>>
>> Example of tc mirror command using hardware:
>> tc filter add dev ens1f0np0 ingress protocol ip prio 1 flower
>> src_mac b4:96:91:a5:c7:a7 skip_sw action mirred egress mirror dev eth1
>>
>> ens1f0np0 - PF
>> b4:96:91:a5:c7:a7 - source MAC address
>> eth1 - PR of a VF to mirror to
>>
>> Signed-off-by: Andrii Staikov <andrii.staikov@...el.com>
> 
> The amount of patches that IMHO should land only into intel-specific
> MLs and instead reaches also netdev, recently increased.

Let's clarify what you mean by "intel-specific MLs".
Do you mean our internal MLs and review or the open one, IWL?

IWL is mostly rudimentary. It's open, but almost nobody outside of Intel
sits there, which means 2/3 of patches doesn't get enough attention and
reviews. It's used by our validation as well, but that's it.
Our internal ML for Ethernet patches works as usually. I realize roughly
half of all patches pass it without a Reviewed-by tag and it's something
we're actively working on. If all goes well, no patches without a proper
review will go outside Intel's internal Ethernet MLs.
Now, the second part,

> 
> Please try harder to apply proper constraints to your traffic, netdev
> is already busy enough!

Do you want us to stop CCing netdev when we send patches to the outer
review to IWL?
This would mean they will once again start missing enough attention from
the outside. I hope you don't want our patches to be reviewed *only* by
Intel folks, right? I don't feel this a good idea.
That's why we started CCing netdev this year. And we do that when we
send patches to IWL, i.e. outside. It's not like "ok, let's Cc netdev
instead of going through our internal review process".

Our clients, partners (e.g. Czech RedHat), our developers, want our
patches to have proper complex review. Dropping netdev would mean that a
patch of some non-corpo guy will be reviewed more carefully and at the
end will have better quality than an Intel patch, which "shouldn't
overburden netdev".
Saying "we'll see them when Tony sends a PR" also doesn't work well for
me. A patch gets taken into a PR once it passes internal review, then
validation, this always do take a while. Imagine waiting for a month for
your patch to be sent in a PR to get a negative review, so that you have
to repeat this process again and wait for another month to get some more
change requests and again :D

In a couple months, no our patches will hit netdev without a proper
Reviewed-by obtained during the internal review, let's not take corner
cases and effectively hide our code from the world?
I don't think you'd like to put a huge banner on netdev's lore saying
"please also take a look at intel-wired-lan" :z I also don't want ppl to
behave like Greg KH some time ago when he said "where's your damn
internal RB, stop abusing LKML" in reply to my early RFC PoC sent only
for an open discussion xD

> 
> Thanks,
> 
> Paolo

Thanks,
Olek

Powered by blists - more mailing lists