Hi Jingjing, Ok! Let's get back to this patch after 1.7 release.
Thanks! Regards, Vladimir 2014-06-14 5:00 GMT+04:00 Wu, Jingjing <jingjing.wu at intel.com>: > Hi, Vladimir > > > > Yes, for Fortville, uint8_t is not enough, it was also the concern is to > keep consistent with flow director?s implementation. But I agree that we > need to change. > > > > Let make an agreement like: > > > > I will make change for the remarks from you. One is the change the type of > rx_queue to uint16_t. The other is change API like > ?rte_eth_dev_add_syn_filter(uint8_t port_id, struct rte_syn_filter *filter, > uint16_t rx_queue)?. > > > > And about the pool and virtualization case, maybe you will send a new > patch about it, maybe me. Whatever, just leave it in future, not include > in this patch. > > > > Thank you! > > Jingjing > > > > *From:* Vladimir Medvedkin [mailto:medvedkinv at gmail.com] > *Sent:* Saturday, June 14, 2014 12:19 AM > *To:* Wu, Jingjing > *Cc:* Thomas Monjalon; dev at dpdk.org > > *Subject:* Re: [dpdk-dev] [PATCH v2 0/4] NIC filters support for generic > filter > > > > 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 at 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 at gmail.com] > *Sent:* Friday, June 13, 2014 7:52 PM > *To:* Thomas Monjalon > *Cc:* Wu, Jingjing; dev at 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 at 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 > > > > >
