| Message ID | 20230912092952.2814966-1-andrii.staikov@intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [iwl-next,v2] ice: Add support for packet mirroring using hardware in switchdev mode | expand |
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@intel.com> The amount of patches that IMHO should land only into intel-specific MLs and instead reaches also netdev, recently increased. Please try harder to apply proper constraints to your traffic, netdev is already busy enough! Thanks, Paolo
From: Paolo Abeni <pabeni@redhat.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@intel.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
On Tue, 2023-09-12 at 16:41 +0200, Alexander Lobakin wrote: > From: Paolo Abeni <pabeni@redhat.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@intel.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 I was under the impression that some patches landed on IWL cc-ing netdev possibly unintentionally, e.g.: https://lore.kernel.org/netdev/20230904021455.3944605-1-junfeng.guo@intel.com/ My intention was to raise attention on such events. Cheers, Paolo
> -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of > Andrii Staikov > Sent: Tuesday, September 12, 2023 3:00 PM > To: intel-wired-lan@lists.osuosl.org > Cc: netdev@vger.kernel.org; Staikov, Andrii <andrii.staikov@intel.com> > Subject: [Intel-wired-lan] [PATCH iwl-next v2] ice: Add support for packet > mirroring using hardware in switchdev mode > > 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@intel.com> > --- > v1 -> v2: no need for changes in ice_add_tc_flower_adv_fltr() > --- > drivers/net/ethernet/intel/ice/ice_switch.c | 25 +++++++++++++++------ > drivers/net/ethernet/intel/ice/ice_tc_lib.c | 13 +++++++++++ > drivers/net/ethernet/intel/ice/ice_type.h | 1 + > 3 files changed, 32 insertions(+), 7 deletions(-) > Tested this patch with HW offload enabled on both PF and VF. Packet mirroring is working on the PF but it fails on the packets originating from VF. Configured below mirror rules for mirroring the packets from both PF1 and VF1 to VF2. tc filter add dev $PF1 ingress protocol ip prio 1 flower src_mac b4:96:91:9f:64:08 skip_sw action mirred egress mirror dev $VF2_PR tc filter add dev $VF1_PR ingress protocol ip prio 1 flower src_mac 52:54:00:00:16:01 skip_sw action mirred egress mirror dev $VF2_PR With the above mirror rules, the packets from PF got mirrored to VF2. But packets originating from VF1 are not seen in VF2.
diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c index 2f77b684ff76..d915b72e8dbb 100644 --- a/drivers/net/ethernet/intel/ice/ice_switch.c +++ b/drivers/net/ethernet/intel/ice/ice_switch.c @@ -6100,6 +6100,7 @@ ice_add_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups, rinfo->sw_act.fltr_act == ICE_FWD_TO_Q || rinfo->sw_act.fltr_act == ICE_FWD_TO_QGRP || rinfo->sw_act.fltr_act == ICE_DROP_PACKET || + rinfo->sw_act.fltr_act == ICE_MIRROR_PACKET || rinfo->sw_act.fltr_act == ICE_NOP)) { status = -EIO; goto free_pkt_profile; @@ -6112,9 +6113,11 @@ ice_add_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups, } if (rinfo->sw_act.fltr_act == ICE_FWD_TO_VSI || - rinfo->sw_act.fltr_act == ICE_NOP) + rinfo->sw_act.fltr_act == ICE_MIRROR_PACKET || + rinfo->sw_act.fltr_act == ICE_NOP) { rinfo->sw_act.fwd_id.hw_vsi_id = ice_get_hw_vsi_num(hw, vsi_handle); + } if (rinfo->src_vsi) rinfo->sw_act.src = ice_get_hw_vsi_num(hw, rinfo->src_vsi); @@ -6150,12 +6153,15 @@ ice_add_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups, status = -ENOMEM; goto free_pkt_profile; } - if (!rinfo->flags_info.act_valid) { - act |= ICE_SINGLE_ACT_LAN_ENABLE; - act |= ICE_SINGLE_ACT_LB_ENABLE; - } else { - act |= rinfo->flags_info.act & (ICE_SINGLE_ACT_LAN_ENABLE | - ICE_SINGLE_ACT_LB_ENABLE); + + if (rinfo->sw_act.fltr_act != ICE_MIRROR_PACKET) { + if (!rinfo->flags_info.act_valid) { + act |= ICE_SINGLE_ACT_LAN_ENABLE; + act |= ICE_SINGLE_ACT_LB_ENABLE; + } else { + act |= rinfo->flags_info.act & (ICE_SINGLE_ACT_LAN_ENABLE | + ICE_SINGLE_ACT_LB_ENABLE); + } } switch (rinfo->sw_act.fltr_act) { @@ -6182,6 +6188,11 @@ ice_add_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups, act |= ICE_SINGLE_ACT_VSI_FORWARDING | ICE_SINGLE_ACT_DROP | ICE_SINGLE_ACT_VALID_BIT; break; + case ICE_MIRROR_PACKET: + act |= ICE_SINGLE_ACT_OTHER_ACTS; + act |= (rinfo->sw_act.fwd_id.hw_vsi_id << ICE_SINGLE_ACT_MIRROR_VSI_ID_S) & + ICE_SINGLE_ACT_MIRROR_VSI_ID_M; + break; case ICE_NOP: act |= FIELD_PREP(ICE_SINGLE_ACT_VSI_ID_M, rinfo->sw_act.fwd_id.hw_vsi_id); diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c index 37b54db91df2..db34df1890f7 100644 --- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c @@ -659,6 +659,19 @@ ice_eswitch_tc_parse_action(struct ice_tc_flower_fltr *fltr, break; + case FLOW_ACTION_MIRRED: + fltr->action.fltr_act = ICE_MIRROR_PACKET; + + if (ice_is_port_repr_netdev(act->dev)) { + repr = ice_netdev_to_repr(act->dev); + + fltr->dest_vsi = repr->src_vsi; + } else { + NL_SET_ERR_MSG_MOD(fltr->extack, "Provided netdevice doesn't support mirroring"); + return -EINVAL; + } + break; + default: NL_SET_ERR_MSG_MOD(fltr->extack, "Unsupported action in switchdev mode"); return -EINVAL; diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h index 02db9e5810e6..f5c35dc8766f 100644 --- a/drivers/net/ethernet/intel/ice/ice_type.h +++ b/drivers/net/ethernet/intel/ice/ice_type.h @@ -1047,6 +1047,7 @@ enum ice_sw_fwd_act_type { ICE_FWD_TO_Q, ICE_FWD_TO_QGRP, ICE_DROP_PACKET, + ICE_MIRROR_PACKET, ICE_NOP, ICE_INVAL_ACT };
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@intel.com> --- v1 -> v2: no need for changes in ice_add_tc_flower_adv_fltr() --- drivers/net/ethernet/intel/ice/ice_switch.c | 25 +++++++++++++++------ drivers/net/ethernet/intel/ice/ice_tc_lib.c | 13 +++++++++++ drivers/net/ethernet/intel/ice/ice_type.h | 1 + 3 files changed, 32 insertions(+), 7 deletions(-)