From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?iso-8859-1?Q?N=E9lio?= Laranjeiro Subject: Re: [PATCH v2 12/20] net/mlx5: add mark/flag flow action Date: Fri, 6 Jul 2018 10:23:19 +0200 Message-ID: <20180706082319.phmy3ri4zvxxe4u6@laranjeiro-vm.dev.6wind.com> References: <90a73b5f33e147ffa3a668f5d19410de17f96045.1530111623.git.nelio.laranjeiro@6wind.com> <20180704083418.GA45405@minint-98vp2qg> <20180705084735.rrgqf3v7dpmo5col@laranjeiro-vm.dev.6wind.com> <20180705195607.GA47821@yongseok-MBP.local> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Cc: dev@dpdk.org, Adrien Mazarguil To: Yongseok Koh Return-path: Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by dpdk.org (Postfix) with ESMTP id E42DD1C474 for ; Fri, 6 Jul 2018 10:23:19 +0200 (CEST) Received: by mail-wr1-f65.google.com with SMTP id p1-v6so3293108wrs.9 for ; Fri, 06 Jul 2018 01:23:19 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20180705195607.GA47821@yongseok-MBP.local> 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 Thu, Jul 05, 2018 at 12:56:09PM -0700, Yongseok Koh wrote: >[...] > > > > + if (mark->id >= MLX5_FLOW_MARK_MAX) > > > > + return rte_flow_error_set(error, EINVAL, > > > > + RTE_FLOW_ERROR_TYPE_ACTION_CONF, > > > > + &mark->id, > > > > + "mark must be between 0 and" > > > > + " 16777199"); > > > > > > Use %d and (MLX5_FLOW_MARK_MAX - 1), instead of fixed string. > > > > It needs an snprintf, rte_flow_error_set() does not accept formatting > > strings. > > I think the following would work but never mind. I'm okay with leaving it as is. > No need to make a change here. > > #define STRINGIFY(x) #x > #define TOSTRING(x) STRINGIFY(x) > "mark must be between 0 and " > TOSTRING(MLX5_FLOW_MARK_MAX - 1)); > It was to avoid adding a macro, but indeed there is the same kind in mlx4, I'll port them for mlx5. > > >[...] > > Addressing both question here, for the flow_stop() and flow_destroy() > > the process is different, for the stop, the flow remains with the mark > > bit set but all queues must me cleared, there is no comparison to make. > > As you can see, it don't even get a flow, it directly unset the mask bit > > in the Rx queues. > > For the destroy the issue is different, several flows may be using the > > same Rx queues, if one of them will remains and has a mark, then the > > associated queues must keep their mark bit set. > > As the process is different, it would end in two distinct functions and > > each one used by a single function. > > > > For the mlx5_flow_rxq_mark(), the situation is different, the same > > process is make when a flow is created and the flow are started. > > I knew the differences but I just wanted to ask if having a separate function > can be a viable option, e.g., > > mlx5_flow_rxq_mark_set() > mlx5_flow_rxq_mark_clear() > mlx5_flow_rxq_mark_trim() Certainly, the point is those functions have a short life as few patches letter they will be removed. I suppose you prefer to have them and I don't think it will take too much time to add such function, it will make part of the next revision ;). Thanks, -- Nélio Laranjeiro 6WIND