From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <medvedkinv@gmail.com> Received: from mail-qc0-f182.google.com (mail-qc0-f182.google.com [209.85.216.182]) by dpdk.org (Postfix) with ESMTP id B7C81B16F for <dev@dpdk.org>; Fri, 13 Jun 2014 18:18:50 +0200 (CEST) Received: by mail-qc0-f182.google.com with SMTP id m20so4357106qcx.27 for <dev@dpdk.org>; Fri, 13 Jun 2014 09:19:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=VeLt5dEqvHT5ihD3uHqsvYeZ/qwnQ1ScNmy/C4dauow=; b=e+C7y9XnP9TBmdq3ah0larNKDmrPkCYoChGxSr9YcRbjdTaK0BmIf9qXPt7Qphl/ng Wgc5GVwkWxzDlkgfjsfDWy/uDz87YdgnVfSo4stTTGEUJEBQaoY8loru/bFVn5ymUFTG AILXZ0gDCHjAvE58dHyEqsM2gkOnJ6MOIMBhSHOv8R6Ac9hxOawnroUuIdeXlu/YvRke eLgGLn3azBpTwo0pIaLX8JpKkB0t48JkW80/ic4BeoXAg3kc55w4GusT7pBhk+iBLCex zOGty2TMyW4nbxy21k3hmqDUWl1rF20HsWzBjO/+rcl2UxpHv//mge9jopRR6elKm1+s fw6w== MIME-Version: 1.0 X-Received: by 10.140.81.146 with SMTP id f18mr5060802qgd.47.1402676345982; Fri, 13 Jun 2014 09:19:05 -0700 (PDT) Received: by 10.140.30.55 with HTTP; Fri, 13 Jun 2014 09:19:05 -0700 (PDT) In-Reply-To: <9BB6961774997848B5B42BEC655768F8A8D402@SHSMSX104.ccr.corp.intel.com> References: <1400895442-32433-1-git-send-email-jingjing.wu@intel.com> <2116871.SYCYqgDKRI@xps13> <9BB6961774997848B5B42BEC655768F8A8BBB5@SHSMSX104.ccr.corp.intel.com> <27520622.tiYQNKLyvh@xps13> <CANDrEH=8SeES7N5eybWaCvJdE0JkZpq-DuxSVYdXAXbm=Wf-Ww@mail.gmail.com> <9BB6961774997848B5B42BEC655768F8A8D402@SHSMSX104.ccr.corp.intel.com> Date: Fri, 13 Jun 2014 20:19:05 +0400 Message-ID: <CANDrEHkPvqmkA6vUukApzTfwJAWz6_R8Lf+RBcJq1J6UNq0uMw@mail.gmail.com> From: Vladimir Medvedkin <medvedkinv@gmail.com> To: "Wu, Jingjing" <jingjing.wu@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Cc: "dev@dpdk.org" <dev@dpdk.org> Subject: Re: [dpdk-dev] [PATCH v2 0/4] NIC filters support for generic filter X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> X-List-Received-Date: Fri, 13 Jun 2014 16:18:51 -0000 Hi Jingjing, Yes, I agree. I have one more remark. It is about type of rx_queue arg. Now it is uint8_t. I think we have to change it to uint16_t because for Fortville NIC it is not enough. Quote fro the datasheet: A PF VSI (Virtual Station Interfaces aka virtual NICs) can allocate and use up to 1536 LQPs (LAN queue pairs). Regards, Vladimir 2014-06-13 18:12 GMT+04:00 Wu, Jingjing <jingjing.wu@intel.com>: > Hi, Vladimir > > > > Thanks a lot for your remarks. > > > > Yes, your understanding is correct, in non-IOV mode, we can use 64pool, > per pool can has 2 queues when ETH_MQ_RX_VMDQ_ONLY. While in IOV mode, > current DPDK version makes the number of queue to 1 by default. The pools > logic makes sense, but I didn=E2=80=99t consider it globally with the thi= nking we > can do it in future. I will be great if you can generate a new patch base= d > on mine. Or we can discuss it further? Due to it is close to the feature > deliver time now and much verification work for it, it may not possible t= o > add it in this patch. > > > > In API > > About your first remark, the reason why I didn=E2=80=99t put the queue in= the > filter structure is that the filter contains the fields used for comparis= on > and the queue is acted as result, and another concern is to keep consiste= nt > with flow director=E2=80=99s implementation. > > About your second remark, I will accept it and integrate the change to > patch in new version. > > > > Do your agree my proposal? > > > > > > *From:* Vladimir Medvedkin [mailto:medvedkinv@gmail.com] > *Sent:* Friday, June 13, 2014 7:52 PM > *To:* Thomas Monjalon > *Cc:* Wu, Jingjing; dev@dpdk.org > > *Subject:* Re: [dpdk-dev] [PATCH v2 0/4] NIC filters support for generic > filter > > > > Hi all, > > The 82599 datasheet (p. 284 and p.287) has only recommendations and only > when possible about assign rx queue not used by RSS/DCB. I do not see any > serious restrictions do not assign the rx queue used by RSS/DCB. > > For cases with only 1 queue if I understand correctly this patch > http://dpdk.org/ml/archives/dev/2014-May/002589.html we can init second > queue in pool and assign it by filter. In *ETH_MQ_RX_VMDQ_ONLY* mode > init all possible queues (even if hardware route packets to zero queue in > pools) so there no problem. Moreover, it is not necesary for rx queue to = be > set in the same pool. > > > About genericity. I agree with Jingjing, different controllers have > different definitions for pools or VFs. And it is only Intel controllers! > It is very hard to predict hardware implementation. For example for > Fortville I can not find 5-tuple filters at all. > > > > API. I have several remarks. > > 1. You use rx_queue as separate arg. For example: > > rte_eth_dev_add_ethertype_filter(uint8_t port_id, uint16_t index, struct > rte_ethertype_filter *filter, uint8_t rx_queue) > rte_eth_dev_get_ethertype_filter(uint8_t port_id, uint16_t index, struct > rte_ethertype_filter *filter, uint8_t *rx_queue) > > you can move uint8_t rx_queue into struct rte_ethertype_filter *filter. > > 2. In SYN filter: > rte_eth_dev_add_syn_filter(uint8_t port_id, uint8_t high_pri, uint8_t > rx_queue) > rte_eth_dev_get_syn_filter(uint8_t port_id, struct rte_syn_filter *filter= , > uint8_t *rx_queue) > > In first ADD func you alloc struct rte_syn_filter inside func, but in GET > func you have to alloc struct rte_syn_filter in your app. May be better t= o > do > rte_eth_dev_add_syn_filter(uint8_t port_id, struct rte_syn_filter *filter= , > uint8_t *rx_queue) ? > > > > So, Jingjing made a lot of work, much more then I (igb filters, testpmd > commands). It works the same as mine (not counting pools logic), so let's > integrate it (it's will be great if jingjing change api according to my > remarks). > > > > Regards, > > Vladimir > > > > 2014-06-12 19:36 GMT+04:00 Thomas Monjalon <thomas.monjalon@6wind.com>: > > > 2014-06-11 17:45, Thomas Monjalon: > > > > My main concern is that Vladimir Medvedkin suggested another API and > I'd > > > like you give your opinion about it: > > > http://dpdk.org/ml/archives/dev/2014-June/003053.html > > > It offers pool number in configuration of the filters. > > 2014-06-12 08:08, Wu, Jingjing: > > > The pool field is used in virtualization scenario. It is acting as one = of > > input set during filter matching in ixgbe. > > My patch didn't consider the virtualization scenario in generic filter > > feature. Because in 82599 datasheet, it is recommended to assign rx > queues > > not used by DCB/RSS, that is virtualization without RSS and DCB mode. F= or > > this mode, current DPDK version makes the number of queue to 1 by > default in > > IOV mode. So in this case it makes no sense make pool as a input set an= d > the > > rx queue also need to be set to in this pool, so just keep the consiste= nt > > with flow director who also ignore it in previous version. > > And further E1000/Niantic/Fortville have different definitions for VF, = we > > need to think how to define it more generic. > > And even just need offer pool number in configuration of the filters as > what > > Vladimir did, it also need to verify the interworking with Virtualizati= on > > for different kinds of NICs, and the interworking with DCB and RSS whic= h > is > > not recommended in 82599's datasheet. > > So I think it will be a good choice to implement generic filter > interworking > > with virtualization in future patch. If there is any volunteer to send > patch > > for support this concern later, it will be also cool. > > Vladimir, do you agree with this analysis? > As you suggested another implementation, I need you acknowledgment for th= is > patchset to be integrated. > > Thanks > -- > Thomas > > >