From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id C67B4A055D; Fri, 19 Feb 2021 13:59:30 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 97C1A16090E; Fri, 19 Feb 2021 13:59:30 +0100 (CET) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by mails.dpdk.org (Postfix) with ESMTP id 9A13116090B for ; Fri, 19 Feb 2021 13:59:29 +0100 (CET) IronPort-SDR: mk7lQ2nZgbSjeuLNRcqAPFt6XOuVTrVFN2c12zG29j2S8nsRdFnRfQEq6nrJda9okEfwjVNM4X plNZLkRoz87Q== X-IronPort-AV: E=McAfee;i="6000,8403,9899"; a="181277289" X-IronPort-AV: E=Sophos;i="5.81,189,1610438400"; d="scan'208";a="181277289" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Feb 2021 04:59:28 -0800 IronPort-SDR: XD2ctmXXkdlGXeJvHsKWaXYjb/BaqXviJrLXaIEYUcLG9G5FLRu4wpv3aa6cuUXpf+C/nCVCGd t175dpi41IsA== X-IronPort-AV: E=Sophos;i="5.81,189,1610438400"; d="scan'208";a="386901607" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.251.55]) ([10.213.251.55]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Feb 2021 04:59:26 -0800 To: Steve Yang , dev@dpdk.org Cc: jia.guo@intel.com, haiyue.wang@intel.com, qiming.yang@intel.com, beilei.xing@intel.com, orika@nvidia.com, murphyx.yang@intel.com References: <20201014084131.72035-1-simonx.lu@intel.com> <20201103082809.41149-1-stevex.yang@intel.com> <20201103082809.41149-7-stevex.yang@intel.com> From: Ferruh Yigit X-User: ferruhy Message-ID: <0d6158d1-3364-5fa1-6260-b546ca061da0@intel.com> Date: Fri, 19 Feb 2021 12:59:24 +0000 MIME-Version: 1.0 In-Reply-To: <20201103082809.41149-7-stevex.yang@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [RFC v2 6/6] net/ixgbe: use flow sample to re-realize mirror rule X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 11/3/2020 8:28 AM, Steve Yang wrote: > When set follow sample rule's ratio equal to one, its behavior is same > as mirror-rule, so we can use "flow create * pattern * actions sample *" > to replace old "set port * mirror-rule *" command now. > > The example of mirror rule command mapping to flow management command: > (in below command, port 0 is PF and port 1-2 is VF): > 1) ingress: pf => pf > set port 0 mirror-rule 0 uplink-mirror dst-pool 2 on > or > flow create 0 ingress pattern pf / end \ > actions sample ratio 1 / port_id id 0 / end > 2) egress: pf => pf > set port 0 mirror-rule 0 downlink-mirror dst-pool 2 on > or > flow create 0 egress pattern pf / end \ > actions sample ratio 1 / port_id id 0 / end > 3) ingress: pf => vf 2 > set port 0 mirror-rule 0 uplink-mirror dst-pool 1 on > or > flow create 0 ingress pattern pf / end \ > actions sample ratio 1 / port_id id 2 / end > 4) egress: pf => vf 2 > set port 0 mirror-rule 0 downlink-mirror dst-pool 1 on > or > flow create 0 egress pattern pf / end \ > actions sample ratio 1 / port_id id 2 / end > 5) ingress: vf 0,1 => pf > set port 0 mirror-rule 0 pool-mirror-up 0x3 dst-pool 2 on > or > flow create 0 ingress pattern vf id is 0 / end \ > actions sample ratio 1 / port_id id 0 / end > flow create 0 ingress pattern vf id is 1 / end \ > actions sample ratio 1 / port_id id 0 / end > 6) ingress: vf 0 => vf 1 > set port 0 mirror-rule 0 pool-mirror-up 0x1 dst-pool 1 on > or > flow create 0 ingress pattern vf id is 0 / end \ > actions sample ratio 1 / port_id id 2 / end > 7) ingress: vlan 4,6 => vf 1 > rx_vlan add 4 port 0 vf 0xf > rx_vlan add 6 port 0 vf 0xf > set port 0 mirror-rule 0 vlan-mirror 4,6 dst-pool 1 on > or > rx_vlan add 4 port 0 vf 0xf > rx_vlan add 6 port 0 vf 0xf > flow create 0 ingress pattern vlan vid is 4 / end \ > actions sample ratio 1 / port_id id 2 / end > flow create 0 ingress pattern vlan vid is 6 / end \ > actions sample ratio 1 / port_id id 2 / end > > Signed-off-by: Steve Yang <...> > +static int > +ixgbe_config_mirror_filter_add(struct rte_eth_dev *dev, > + struct ixgbe_flow_mirror_conf *mirror_conf) > +{ > + struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev); > + uint32_t mr_ctl, vlvf; > + uint32_t mp_lsb = 0; > + uint32_t mv_msb = 0; > + uint32_t mv_lsb = 0; > + uint32_t mp_msb = 0; > + uint8_t i = 0; > + int reg_index = 0; > + uint64_t vlan_mask = 0; > + > + const uint8_t pool_mask_offset = 32; > + const uint8_t vlan_mask_offset = 32; > + const uint8_t dst_pool_offset = 8; > + const uint8_t rule_mr_offset = 4; > + const uint8_t mirror_rule_mask = 0x0F; > + > + struct ixgbe_hw *hw = > + IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); > + struct ixgbe_filter_info *filter_info = > + IXGBE_DEV_PRIVATE_TO_FILTER_INFO(dev->data->dev_private); > + struct ixgbe_mirror_conf_ele *it; > + int8_t rule_id; > + uint8_t mirror_type = 0; > + > + if (ixgbe_vt_check(hw) < 0) > + return -ENOTSUP; > + > + if (IXGBE_INVALID_MIRROR_TYPE(mirror_conf->rule_type)) { > + PMD_DRV_LOG(ERR, "unsupported mirror type 0x%x.", > + mirror_conf->rule_type); > + return -EINVAL; > + } > + > + TAILQ_FOREACH(it, &filter_mirror_list, entries) { > + if (it->filter_info.rule_type == mirror_conf->rule_type && > + it->filter_info.dst_pool == mirror_conf->dst_pool && > + it->filter_info.pool_mask == mirror_conf->pool_mask && > + it->filter_info.vlan_mask == mirror_conf->vlan_mask && > + !memcmp(it->filter_info.vlan_id, mirror_conf->vlan_id, > + ETH_MIRROR_MAX_VLANS * sizeof(mirror_conf->vlan_id[0]))) { > + PMD_DRV_LOG(ERR, "mirror rule exists."); > + return -EEXIST; > + } > + } Can you please help me to understand this. There is a per device 'ixgbe_filter_info->mirror_filters', also there is a driver global 'ixgbe_mirror_filter_list', When a new filter added, above checks the 'ixgbe_mirror_filter_list' for all filters, but what if driver is driving multiple devices and we want to add same rule for each device, won't above checks be wrong? Btw, I am not clear why there are two separate per-device and driver global list exists, I can see that is a common patter for filters not specific to this one, can you please explain? <...> > +static void > +ixgbe_clear_all_mirror_filter(struct rte_eth_dev *dev) > +{ > + struct ixgbe_filter_info *filter_info = > + IXGBE_DEV_PRIVATE_TO_FILTER_INFO(dev->data->dev_private); > + int i; > + > + for (i = 0; i < IXGBE_MAX_MIRROR_RULES; i++) { > + if (filter_info->mirror_mask & (1 << i)) { > + ixgbe_config_mirror_filter_del(dev, > + &filter_info->mirror_filters[i]); > + } > + } This function is clearing all device filters, but not removing them from global list, is this intentional? > +} > + > void > ixgbe_filterlist_init(void) > { > @@ -3185,6 +3365,7 @@ ixgbe_filterlist_init(void) > TAILQ_INIT(&filter_fdir_list); > TAILQ_INIT(&filter_l2_tunnel_list); > TAILQ_INIT(&filter_rss_list); > + TAILQ_INIT(&filter_mirror_list); > TAILQ_INIT(&ixgbe_flow_list); > } > > @@ -3198,6 +3379,7 @@ ixgbe_filterlist_flush(void) > struct ixgbe_fdir_rule_ele *fdir_rule_ptr; > struct ixgbe_flow_mem *ixgbe_flow_mem_ptr; > struct ixgbe_rss_conf_ele *rss_filter_ptr; > + struct ixgbe_mirror_conf_ele *mirror_filter_ptr; > > while ((ntuple_filter_ptr = TAILQ_FIRST(&filter_ntuple_list))) { > TAILQ_REMOVE(&filter_ntuple_list, > @@ -3241,6 +3423,13 @@ ixgbe_filterlist_flush(void) > rte_free(rss_filter_ptr); > } > > + while ((mirror_filter_ptr = TAILQ_FIRST(&filter_mirror_list))) { > + TAILQ_REMOVE(&filter_mirror_list, > + mirror_filter_ptr, > + entries); > + rte_free(mirror_filter_ptr); > + } This block removes filters from global list, but it doesn't remove them from per-device list, and it doesn't remove them from device, so the filtering still be active in the device after this call, is this intentional? <...> > + memset(&mirror_conf, 0, sizeof(struct ixgbe_flow_mirror_conf)); > + ret = ixgbe_parse_sample_filter(dev, attr, pattern, > + actions, &mirror_conf, error); > + if (!ret) { > + /* Just support mirror behavior */ > + ret = ixgbe_config_mirror_filter_add(dev, &mirror_conf); > + if (ret) { > + PMD_DRV_LOG(ERR, "failed to add mirror filter"); > + goto out; > + } > + > + mirror_filter_ptr = rte_zmalloc("ixgbe_mirror_filter", > + sizeof(struct ixgbe_mirror_conf_ele), 0); > + if (!mirror_filter_ptr) { > + PMD_DRV_LOG(ERR, "failed to allocate memory"); > + goto out; Should rollback the above filter addition on the error?