Re: [dpdk-dev] [PATCH v2 0/4] NIC filters support for generic filter

[Vladimir Medvedkin <medvedkinv@gmail.com>]

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’t consider it globally with the thinking we
> can do it in future. I will be great if you can generate a new patch based
> 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 to
> add it in this patch.
>
>
>
> In API
>
> About your first remark, the reason why I didn’t put the queue in the
> filter structure is that the filter contains the fields used for comparison
> and the queue is acted as result, and another concern is to keep consistent
> with flow director’s 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 to
> 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. For
> > 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 and
> the
> > rx queue also need to be set to in this pool, so just keep the consistent
> > 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 Virtualization
> > for different kinds of NICs, and the interworking with DCB and RSS which
> 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 this
> patchset to be integrated.
>
> Thanks
> --
> Thomas
>
>
>