From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by dpdk.org (Postfix) with ESMTP id 05AAD1BE7C for ; Fri, 6 Jul 2018 17:59:07 +0200 (CEST) Received: by mail-wr1-f68.google.com with SMTP id h10-v6so4674047wrq.8 for ; Fri, 06 Jul 2018 08:59:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=RSMkaFbmSBl3Jd894L93BjR850rYLVAOcVjMhWPpGsU=; b=tQ+E4ujKYlayuoGNyeiWe7kzJg/EU/QuppbsxnjTc+rMS1ewfY4Q69wcK9EeTnJUK4 I0z+TPvz53PGDK3OsZjg+zqyOM4DTXQN7IbE4bKiZf/72Z5PS8NeYffCHRY/9Dwj8wPy zwCRl1idK/fUL+LS5lyefrKlNJuUdrrlLvg4iXOWb6HoZSEpFNAouofZdYo6j38CbJtH i8rjgHEEdNADFr5cRvCrb4/ktHnC98ZpRPP/RoFJBODkc6KGZsnDnMmSr9QLRRjdOY/F lLnzMfejSKW9NrEteeQbHFPmu0IgcZqIMsmaMmhdiKMgeiQBTHWh19nOX4Ipmnr/OjHF jLDQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=RSMkaFbmSBl3Jd894L93BjR850rYLVAOcVjMhWPpGsU=; b=JRGxh4mGT5Xq86U9k0WrYTcyy2PMIAv8Xi+ftdjIldRDT8pE2xsv4zQSZqxHWlOoYz s0G8JyQGomVuyFpggRsXMIj+FYA6xeGZGCPUL/SizV/023jSRFxnWofCCFqnN6Q49C/R DO16oEXAf4VR8K2VLJDQbChITsO+JUER8nMVTNmhwmEV6j99duKm2b98J0lK2BrmckLB bVAMnRwzzkh5qV8LftuGcc7+0yzcnrbVrVcw0dFKcthAn44pycL8rIcFho7nP3Ok3quY y8Wgw4eFJ2GE5Y7SQT2u3hXzqfRgLLhEkke7cj13KLq9fAaS0CMlWBjou+c59fMDMQ1C SBIw== X-Gm-Message-State: APt69E1/ihEm1sxHpVPs5YRAyfkTutlUCpP3cP3wJyvxlrVNCoU6wK9S PCHUTxVp3XMRGb7S+GrfJVuz X-Google-Smtp-Source: AAOMgpe0a9gpnmbwoj42UyjKtSfK/NAtiNIRTNwFrB8IMZtpvEwfkzZokcKMzum34pbnWFZbJALpdQ== X-Received: by 2002:adf:967d:: with SMTP id c58-v6mr7827679wra.197.1530892747663; Fri, 06 Jul 2018 08:59:07 -0700 (PDT) Received: from laranjeiro-vm.dev.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id p184-v6sm5427457wmp.31.2018.07.06.08.59.06 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 06 Jul 2018 08:59:06 -0700 (PDT) Date: Fri, 6 Jul 2018 17:59:07 +0200 From: =?iso-8859-1?Q?N=E9lio?= Laranjeiro To: Yongseok Koh Cc: dev@dpdk.org, Adrien Mazarguil Message-ID: <20180706155907.x7geq7jxmbzs76im@laranjeiro-vm.dev.6wind.com> References: <20180706021630.GB47821@yongseok-MBP.local> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180706021630.GB47821@yongseok-MBP.local> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v2 13/20] net/mlx5: add RSS flow action X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 Jul 2018 15:59:08 -0000 Hi Yongseok, I am only addressing your questions concerns here, almost all other points I also agree with them. On Thu, Jul 05, 2018 at 07:16:35PM -0700, Yongseok Koh wrote: > On Wed, Jun 27, 2018 at 05:07:45PM +0200, Nelio Laranjeiro wrote: > > Signed-off-by: Nelio Laranjeiro > > --- >[...] > > > + */ > > +static void > > +mlx5_flow_layers_update(struct rte_flow *flow, uint32_t layers) > > +{ > > + if (flow->expand) { > > + if (flow->cur_verbs) > > + flow->cur_verbs->layers |= layers; > > If flow->cur_verbs is null, does that mean it is a testing call? Then, is it > unnecessary to update layers for the testing call? Confusing.. No it may also happen if the buffer was too small, in any case the code continues its validation. > > + } else { > > + flow->layers |= layers; > > + } > > +} > > + > > +/** > > + * Get layers bit-field. > > + * > > + * @param flow[in, out] > > + * Pointer to flow structure. > > + */ > > +static uint32_t > > +mlx5_flow_layers(struct rte_flow *flow) > > +{ > > + uint32_t layers = flow->layers; > > + > > + if (flow->expand && flow->cur_verbs) > > If flow is expanded and it is a testing call, then flow->layers is used? > > > + layers |= flow->cur_verbs->layers; > > + return layers; > > This part is so unclear to me, hard to understand. There are two 'layers' > fields, one in rte_flow and the other in mlx5_flow_verbs. It seems > rte_flow->layers is used only when the flow isn't expanded. If a flow is > expanded, flow->expand is set after processing the first entry in the expanded > list. In mlx5_flow_merge(), > > for (i = 0; i != buf->entries; ++i) { > > ... > > flow->expand = !!(buf->entries > 1); > } > > Why is flow->expand set at the end of the loop? Is this in order to avoid > validation for the expanded flows? > mlx5_flow_item_xxx() executes validation only if flow->expand is zero, > why? Expanded flows are PMD internal cooking to match the user request, they are fully added by the PMD and thus must be valid. There is not need to validate their position nor redundancy and for the spec, last, mask they are provided as wildcards which means those pointer are null. > And why does mlx5_flow_layers() have to return (flow->layers | > flow->cur_verbs->layers) if expanded? You are right, and indeed it is a bug. With the fix, only the expanded flow will use the verbs->layers, whereas the first conversion will use the flow->layers. > If there are 3 entries in the rte_flow_expand_rss, > eth > eth / ipv4 / udp > eth / ipv6 / udp > > Then, the 2nd and 3rd don't have MLX5_FLOW_LAYER_OUTER_L2 in layers field? > Please explain in details and add comments appropriately. The 2nd and 3rd should have the same layers bits as the 1st rule in addition of their own bits set .e.g 2nd rule will have L2 + L3_IPv4 + L4_UDP the 3rd L3 + L3_IPv6 + L4_UDP. I will try to sanitize it, the issue being for a single rte_flow we can have several verbs flows with the expansion, for each of them we need to update the priority and this based on the more specific layer of each, it needs to be stored in some place. I think the biggest issue you are raising here is the lack of correct variable naming and documentation, I'll try to improve it at most. >[...] > > + if (verbs) { > > + verbs->modifier |= MLX5_FLOW_MOD_MARK; > > + if (verbs->modifier & MLX5_FLOW_MOD_FLAG) { > > + mlx5_flow_verbs_mark_update(verbs, mark->id); > > + size = 0; > > + } else if (size <= flow_size) { > > If verbs isn't null (not testing call), isn't it guaranteed there's enough > space? Is it still needed to check the size? Unfortunately not, verbs variable may be valid and pointing into a zone in the buffer but it does not mean there is enough space to store the mark specification in the buffer. > > tag.tag_id = mlx5_flow_mark_set(mark->id); > > mlx5_flow_spec_verbs_add(flow, &tag, size); > > } > > + } else { > > + if (flow->modifier & MLX5_FLOW_MOD_FLAG) > > + size = 0; > > } > > flow->modifier |= MLX5_FLOW_MOD_MARK; > > return size; > > @@ -1185,6 +1656,15 @@ mlx5_flow_actions(struct rte_eth_dev *dev, > > int remain = flow_size; > > int ret = 0; > > > > + /* > > + * FLAG/MARK are the only actions having a specification in Verbs and > > + * not making part of the packet fate. Due to this specificity and to > > + * avoid extra variable, their bit in the flow->modifier bit-field are > > + * disabled here to compute the exact necessary memory those action > > + * needs. > > + */ > > + flow->modifier &= ~(MLX5_FLOW_MOD_FLAG | MLX5_FLOW_MOD_MARK); > > Can't understand this well. Is this for the case where the flow is expanded? If > so, why don't you reset flow->modifier in the for loop of mlx5_flow_merge()? Yes it is, I'll move it. >[...] > > + assert(ret > 0); > > + buf = rte_calloc(__func__, 1, ret, 0); > > + if (!buf) { > > + rte_flow_error_set(error, ENOMEM, > > + RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > > + NULL, > > + "not enough memory to expand the RSS flow"); > > + goto error; > > + } > > I'm pretty sure you've already fixed this bug. Validation can't return ENOMEM. You know me well ;) Thanks, -- Nélio Laranjeiro 6WIND