From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f46.google.com (mail-wm0-f46.google.com [74.125.82.46]) by dpdk.org (Postfix) with ESMTP id 840F12A5B for ; Tue, 30 May 2017 14:46:37 +0200 (CEST) Received: by mail-wm0-f46.google.com with SMTP id e127so99389257wmg.1 for ; Tue, 30 May 2017 05:46:37 -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:in-reply-to; bh=iy+WuRDpvYwKBlS1oz22V72uscgOqyi87nqd81oaF70=; b=LfinSmB1DOubMoENZJoVds6a1cq6E+SjA3bpLM4JQwOYgxnMadgyiKrw/H/POR2sMk tN8YV/AnMIJFsc4fRG4NMjrVGLf8NT50KahYUhSOJ/yz9IXbJhPWm+P7cV+O2x/FNOtV 8cal7gj9V+1m6pGfGgcnNnZq1ziAb+P2NogXauK3Rm7LDxB1zBS1iKnKcfLSau4WCLyR Ox+ej4Oxwf5uwiyftY2XGI95DrlCbJVB5QX+v6QWnGxWMaJR0z2p190r+z555iZRlJni Mumc9eZaWE5PtaPcU4Tfgb4ff5K8xKL4s9yQBRDDhimnn5dviq2a2/uTW55UivzpotEE WJ9Q== 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:in-reply-to; bh=iy+WuRDpvYwKBlS1oz22V72uscgOqyi87nqd81oaF70=; b=QO0z5kgjZKk9AGVszA9iCvScwSklD+CoBGVTCHeGf1PMIfGzI1V73dJb4z7VJBf3xW EPzcW82as3Cd6Z54ZCVvNNGxe3YRh9BAr7Vfq+cZfVDGHztvdy0uXGT28ttRQ4DtOKlU KaPJii6ZNJjwmD57zyVCP01hhq3ALu6cBr45kztiEpY5AS4EbVb/1+F2cr5f1bjribQw OJ6Nrx1oVVScSkS+jtMNdD1Of7hw044Ox1SIM0dt24e8futl3Hp9RHqim61PDmRpF2uq 5vSfyi06KYsYqBHbYXF8YieFrTym2GdJjbS3E0lAJ5wVnZGrzMvzFYNCcTNCD4Mv8AeY YjSw== X-Gm-Message-State: AODbwcA33LMaKWT9NMmcbTtV4z6jPQieP8jj4Crg9BpgwbgIYaimXlWH 7Y4kX0bgh5NEwY7xI8Q= X-Received: by 10.223.152.227 with SMTP id w90mr7490140wrb.151.1496148397103; Tue, 30 May 2017 05:46:37 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id z12sm18722704wrb.41.2017.05.30.05.46.35 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 30 May 2017 05:46:36 -0700 (PDT) Date: Tue, 30 May 2017 14:46:30 +0200 From: Adrien Mazarguil To: Qi Zhang Cc: dev@dpdk.org, "Mcnamara, John" Message-ID: <20170530124630.GA1758@6wind.com> References: <1495582134-13665-1-git-send-email-qi.z.zhang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1495582134-13665-1-git-send-email-qi.z.zhang@intel.com> Subject: Re: [dpdk-dev] [PATCH] ethdev: add roughly match pattern 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: Tue, 30 May 2017 12:46:37 -0000 Hi Zhang, You should cram "flow API" somewhere in the title of such commits. On Tue, May 23, 2017 at 07:28:54PM -0400, Qi Zhang wrote: > Add new meta pattern item RTE_FLOW_TYPE_ITEM_ROUGHLY. > > This is for device that support no-perfect match option. > Usually a no-perfect match is fast but the cost is accuracy. > i.e. Signature Match only match pattern's hash value, but it is > possible two different patterns have the same hash value. > > Matching accuracy level can be configure by subfield threshold. > Driver can divide the range of threshold and map to different > accuracy levels that device support. > > Signed-off-by: Qi Zhang While I really like the "roughly" pattern item name since it perfectly describes its intended purpose in my opinion, perhaps some may not find this name appropriate. I would like to hear other people's opinion on the matter and not be the only one to ack this patch. Several more comments below. > --- > app/test-pmd/cmdline_flow.c | 24 ++++++++++++++ > app/test-pmd/config.c | 1 + > doc/guides/prog_guide/rte_flow.rst | 50 +++++++++++++++++++++++++++++ > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 2 ++ > lib/librte_ether/rte_flow.h | 30 +++++++++++++++++ > 5 files changed, 107 insertions(+) > > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c > index 0fd69f9..18ffcff 100644 > --- a/app/test-pmd/cmdline_flow.c > +++ b/app/test-pmd/cmdline_flow.c > @@ -107,6 +107,8 @@ enum index { > ITEM_END, > ITEM_VOID, > ITEM_INVERT, > + ITEM_ROUGHLY, > + ITEM_ROUGHLY_THRESHOLD, "Threshold" is commonly abbreviated as "thresh", I think it's fine if you use this shorter form here and in the structure definition. There is also an issue with the position of these enum entries. They should come in the same order as rte_flow.h definitions like you did, but you are supposed to add new entries at the end of the various lists in that file instead of in the middle, otherwise you're destroying API/ABI compatibility which is not supposed to happen. More on that topic below. > ITEM_ANY, > ITEM_ANY_NUM, > ITEM_PF, > @@ -426,6 +428,7 @@ static const enum index next_item[] = { > ITEM_END, > ITEM_VOID, > ITEM_INVERT, > + ITEM_ROUGHLY, This will have to be moved at the end of the list. > ITEM_ANY, > ITEM_PF, > ITEM_VF, > @@ -447,6 +450,12 @@ static const enum index next_item[] = { > ZERO, > }; > > +static const enum index item_roughly[] = { > + ITEM_ROUGHLY_THRESHOLD, I suggest "ITEM_ROUGHLY_THRESH". > + ITEM_NEXT, > + ZERO, > +}; > + > static const enum index item_any[] = { > ITEM_ANY_NUM, > ITEM_NEXT, > @@ -954,6 +963,21 @@ static const struct token token_list[] = { > .next = NEXT(NEXT_ENTRY(ITEM_NEXT)), > .call = parse_vc, > }, > + [ITEM_ROUGHLY] = { This will have to be moved at the end of the list. > + .name = "roughly", > + .help = "match the pattern roughly", Hehe, the question is who will go out of their way to match traffic roughly instead of perfectly? They need a better incentive to do so, in a very short sentence. > + .priv = PRIV_ITEM(ROUGHLY, > + sizeof(struct rte_flow_item_roughly)), > + .next = NEXT(item_roughly), > + .call = parse_vc, > + }, > + [ITEM_ROUGHLY_THRESHOLD] = { > + .name = "threshold", "thresh" again, I won't comment them all, you get the idea. > + .help = "match accuracy threshold", > + .next = NEXT(item_roughly, NEXT_ENTRY(UNSIGNED), item_param), > + .args = ARGS(ARGS_ENTRY(struct rte_flow_item_roughly, > + threshold)), > + }, > [ITEM_ANY] = { > .name = "any", > .help = "match any protocol for the current layer", > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c > index 4d873cd..5b0cd4d 100644 > --- a/app/test-pmd/config.c > +++ b/app/test-pmd/config.c > @@ -954,6 +954,7 @@ static const struct { > MK_FLOW_ITEM(END, 0), > MK_FLOW_ITEM(VOID, 0), > MK_FLOW_ITEM(INVERT, 0), > + MK_FLOW_ITEM(ROUGHLY, sizeof(struct rte_flow_item_roughly)), This will have to be moved at the end of the list. > MK_FLOW_ITEM(ANY, sizeof(struct rte_flow_item_any)), > MK_FLOW_ITEM(PF, 0), > MK_FLOW_ITEM(VF, sizeof(struct rte_flow_item_vf)), > diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst > index b587ba9..4cc1876 100644 > --- a/doc/guides/prog_guide/rte_flow.rst > +++ b/doc/guides/prog_guide/rte_flow.rst > @@ -491,6 +491,7 @@ Usage example, matching non-TCPv4 packets only: > > +-------+----------+ > | Index | Item | > + This change looks unnecessary. > +=======+==========+ > | 0 | INVERT | > +-------+----------+ > @@ -503,6 +504,55 @@ Usage example, matching non-TCPv4 packets only: > | 4 | END | > +-------+----------+ > > +Item: ``ROUGHLY`` > +^^^^^^^^^^^^^^^^^ This will have to be moved at the end of the list (documentation also follows the same order as rte_flow.h). > + > +Roughly matching, not perfect match. > + > +This is for device that support no-perfect match option. > +Usually a no-perfect match is fast but the cost is accuracy. > +i.e. Signature Match only match pattern's hash value, but it is > +possible two different patterns have the same hash value. > + > +Matching accuracy level can be configure by threshold. > +Driver can divide the range of threshold and map to different > +accuracy levels that device support. Please expand these paragraphs to fit 75-79 columns wide like the rest of the file. I think a better wording is necessary to provide incentives for applications to use this mode. I have a few ideas but I'm not familiar enough with the original signature mode for that. Perhaps John can help? > + > +.. _table_rte_flow_item_roughly: > + > +.. table:: ROUGHLY > + > + +----------+---------------+--------------------------------------------------+ > + | Field | Subfield | Value | > + +==========+===========+======================================================+ > + | ``spec`` | ``threshold`` | 0 as perfect match, 0xffffffff as roughest match | > + +----------+---------------+--------------------------------------------------+ > + | ``last`` | ``threshold`` | ignored | > + +----------+-----------+------------------------------------------------------+ > + | ``mask`` | ``threshold`` | ignored | > + +----------+-----------+------------------------------------------------------+ Last and mask cannot be ignored. The only items where they are ignored are those that do not even take a spec definition. This means that a mask set to 0 is supposed to be the same as no item provided. PMDs should retrieve the threshold value using something like: thresh = spec->thresh & mask->thresh; if (last->thresh && (last->thresh & mask->thresh) < thresh) complain_unsupported(); Ranges (last) can be ignored when otherwise valid because what matters is only the lowest threshold value. See 8.2.3 Pattern item [1] for more information. > + Extra empty line. > + > +Usage example, roughly match a TCPv4 packets: > + > +.. _table_rte_flow_item_roughly_example: > + > +.. table:: Roughly matching How about "Rough matching". > + > + +-------+----------+ > + | Index | Item | > + +=======+==========+ > + | 0 | ROUGHLY | > + +-------+----------+ > + | 1 | Ethernet | > + +-------+----------+ > + | 2 | IPv4 | > + +-------+----------+ > + | 3 | TCP | > + +-------+----------+ > + | 4 | END | > + +-------+----------+ > + > Item: ``PF`` > ^^^^^^^^^^^^ There is a missing change in this file. You must modify the following statement: - Signature mode of operation is not defined but could be handled through a specific item type if needed. And mention ROUGHLY in index 3 of the table below: +---+-------------------+----------+-----+ | | 3 | VF, PF (optional) | ``spec`` | any | | | | +----------+-----+ | | | | ``last`` | N/A | | | | +----------+-----+ | | | | ``mask`` | any | | +---+-------------------+----------+-----+ | > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > index 0e50c10..08a88f8 100644 > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > @@ -2513,6 +2513,8 @@ This section lists supported pattern items and their attributes, if any. > > - ``invert``: perform actions when pattern does not match. > > +- ``roughly``: pattern is matched roughly. > + How about "match pattern roughly"? (remember to keep this in sync with testpmd's inline help string though) This will also have to be moved at the end of the list. > - ``any``: match any protocol for the current layer. > > - ``num {unsigned}``: number of layers covered. > diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h > index c47edbc..4921858 100644 > --- a/lib/librte_ether/rte_flow.h > +++ b/lib/librte_ether/rte_flow.h > @@ -148,6 +148,18 @@ enum rte_flow_item_type { > RTE_FLOW_ITEM_TYPE_INVERT, > > /** > + * [META] > + * > + * Roughly matching, not perfect matching > + * > + * This is for device that support no-perfect matching option. > + * Usually a no-perfect matching is fast but the cost is accuracy. Perhaps John can help here as well. This one should be kept mostly in sync with the full description for struct rte_flow_item_roughly and also the documentation in rte_flow.rst. > + * > + * See struct rte_flow_item_roughly Missing colon. > + */ > + RTE_FLOW_ITEM_TYPE_ROUGHLY, > + This new item type *must* be moved at the end of the list to avoid breaking API/ABI and existing applications. Remember it's append-only. You then need to update the rest of the patch accordingly. > + /** > * Matches any protocol in place of the current layer, a single ANY > * may also stand for several protocol layers. > * > @@ -300,6 +312,24 @@ enum rte_flow_item_type { > }; > > /** > + * RTE_FLOW_ITEM_TYPE_ROUGHLY > + * > + * Roughly matching, not perfect match. > + * > + * This is for device that support no-perfect match option. > + * Usually a no-perfect match is fast but the cost is accuracy. > + * i.e. Signature Match only match pattern's hash value, but it is > + * possible two different patterns have the same hash value. > + * > + * Matching accuracy level can be configure by threshold. > + * Driver can divide the range of threshold and map to different > + * accuracy levels that device support. John? > + */ > +struct rte_flow_item_roughly { > + uint32_t threshold; /**< accuracy threshold*/ How about: uint32_t thresh; /**< Accuracy threshold. */ > +}; Again this structure must be defined after all the others to keep the same order as enum rte_flow_item_type. > + > +/** > * RTE_FLOW_ITEM_TYPE_ANY > * > * Matches any protocol in place of the current layer, a single ANY may also > -- > 2.7.4 > [1] http://dpdk.org/doc/guides/prog_guide/rte_flow.html#pattern-item -- Adrien Mazarguil 6WIND