Thank you Andrew for the detailed explanation. It is very helpful for me to understand the plugin.
Thanks Yipeng > -----Original Message----- > From: Andrew 👽 Yourtchenko [mailto:ayour...@gmail.com] > Sent: Tuesday, August 29, 2017 5:05 AM > To: Wang, Yipeng1 <yipeng1.w...@intel.com> > Cc: vpp-dev@lists.fd.io; zhang...@yunshan.net.cn > Subject: Re: [vpp-dev] ACL Match in fa_node.c > > Hi Yipeng, > > yeah, this case should be handled as well - note that the ACL lookup hash is > 48x8, > while the session lookup hash is 40x8, and the fa_5tuple_t (being 48 bytes in > size) > contains the fa_packet_info_t which is sitting at the last 8 bytes. > > That data structure has a bunch of flag-type information derived from the > packet: > > typedef union { > u64 as_u64; > struct { > u32 sw_if_index; > u16 mask_type_index_lsb; > u8 tcp_flags; > u8 tcp_flags_valid:1; > u8 is_input:1; > u8 l4_valid:1; > u8 is_nonfirst_fragment:1; > u8 is_ip6:1; > u8 flags_reserved:3; > }; > } fa_packet_info_t; > > They are filled during the initial extraction of the 5-tuple, and are also > used for > the lookup in the hash table (this way we can match non-initial fragments > differently from the initial ones). > > Note the "mask_type_index_lsb" field - when preparing for the lookup iteration > in multi_acl_match_get_applied_ace_index, it is filled with the index of the > mask > used: > > kv_key->pkt.mask_type_index_lsb = mask_type_index; > > So in the example you cite, there will be two lookups in > multi_acl_match_get_applied_ace_index, which will both match, but the one > which is earlier in the ACLs will win, because of the check in "if > (result_val- > >applied_entry_index < curr_match_index)" - thus the end result will be the > same as if we just scanned the entire list downwards. > > Hopefully this clarifies the logic! :-) > > --a > > On 8/29/17, Wang, Yipeng1 <yipeng1.w...@intel.com> wrote: > > Thank you Andrew, seems I was looking at an older version... > > > > I was thinking of similar thing like using tuple space search like > > you already done. It is very good work and lots of considerations. One > > quick > > question: > > > > I just start looking at the code, but it seems all the rules are > > stored in one hash table (acl_lookup_hash). If a packet say having src > > IP= 1.1.0.0, dst=2.2.2.2, and there are two rules one is > > src=1.1.0.0/16, dst=2.2.0.0/32 and the other is src=1.1.0.0/32, > > dst=2.2.0.0/16. The two rules will have same key value in the table > > right? When do lookup, assume packet header is masked by the second rule’s > mask type and become src=1.1.0.0 dst=2.2.0.0. > > Will it match the first rule as well by mistake? > > > > Thank you. > > Yipeng > > > > From: Andrew Yourtchenko [mailto:ayour...@gmail.com] > > Sent: Sunday, August 27, 2017 6:30 AM > > To: Wang, Yipeng1 <yipeng1.w...@intel.com> > > Cc: vpp-dev@lists.fd.io; zhang...@yunshan.net.cn > > Subject: Re: [vpp-dev] ACL Match in fa_node.c > > > > Hi Yipeng, > > > > It's already there - just have a look through hash_* files in the ACL > > plugin directory on the master or latest stable/1707 :-) > > > > There are several things more that can be taken care of (e.g. the > > determination of the "ACE not shadowed" shortcut for applied ACEs > > would allow some good further improvement, or more distinction between > > the v4 and > > v6 lookups, and potentially more cleverness with handling of port ranges). > > > > Also, if you have ideas for a different lookup mechanism - feel free > > to add one - using a similar API as for the hash-based one it should > > be fairly straightforward. > > > > Do you have some specific ideas in mind ? > > > > --a > > > > On 26 Aug 2017, at 03:23, Wang, Yipeng1 > > <yipeng1.w...@intel.com<mailto:yipeng1.w...@intel.com>> wrote: > > Hi, Andrew and Pan, > > > > I came across the thread you are talking about improvements of ACL plugin. > > Any update? I am also interested in the performance of ACL, wondering > > what is the direction of the optimizations you are working on. Could you > > share? > > > > Thanks > > Yipeng > > > > > > > > > >>On 5/23/17, å¼ æ”€ <zhangpan at > >> yunshan.net.cn<https://lists.fd.io/mailman/listinfo/vpp-dev>> wrote: > > > >> Hi Andrew! > > > >> > > > >> > > > >> ------------------ Original ------------------ > > > >> From: "Andrew  Yourtchenko"<ayourtch at > >> gmail.com<https://lists.fd.io/mailman/listinfo/vpp-dev>>; > > > >> Date: Tue, May 23, 2017 07:56 PM > > > >> To: "å¼ æ”€"<zhangpan at > >> yunshan.net.cn<https://lists.fd.io/mailman/listinfo/vpp-dev>>; > > > >> Cc: "vpp-dev"<vpp-dev at > >> lists.fd.io<https://lists.fd.io/mailman/listinfo/vpp-dev>>; > > > >> Subject: Re: [vpp-dev] ACL Match in fa_node.c > > > >> > > > >> > > > >> Hi! > > > >> > > > >> On 5/23/17, å¼ æ”€ <zhangpan at > >> yunshan.net.cn<https://lists.fd.io/mailman/listinfo/vpp-dev>> wrote: > > > >>> Hi guys, > > > >>> > > > >>> > > > >>> I looked into the source code of vpp/src/plugin/acl/fa_node.c, > > > >>> in function full_acl_match_5tuple(), it seems that every ingress > >>> packet > > > >>> is > > > >>> matching against each ACL rule stored in acl_main->acls in a > >>> for-loop > > > >>> manner. This seems not fairly effective. > > > >> > > > >> You're absolutely right on both counts. First make it work, then make > > > >> it right, then make it fast :-) I have some ideas that I wanted to > > > >> experiment with there, would you be interested to help ? ACL matching > > > >> is a fairly distinct function operation to not affect much else. > > > >> > > > >> [PAN]: I would be very pleased to help as I am also a addicted person > >> to > > > >> high performance programming :D > > > >> > > > >> > > > > > > > > Great! :-) I will try to sketch the idea tomorrow/thursday and will > > > > add you to the draft, so we can work together. (I also will have > > > > limited connectivity the next week, so I won't be a lot in your way! > > > > :-) (or if you have some good idea on how you'd like to do it, feel > > > > free to shoot a doc/code into a gerrit draft, let's see!) > > > > > > > >> > > > >>> > > > >>> > > > >>> Besides, I notice that in vpp/src/plugin/acl/acl.c，when you call the > > > >>> function acl_hook_l2_input_classify(), you will create a > > > >>> vnet_classify_table, but I didn't see any code which adds > > > >>> classify_session > > > >>> to it, why? > > > >> > > > >> I had used classify table for storing the sessions in the pre-1704 > > > >> version of the ACL plugin. > > > >> > > > >> in 1704 as I was adding the L3, I moved to the new data path while > > > >> keeping the old one still around, and potentially switchable (not > > > >> terribly gracefully, but still). > > > >> > > > >> In the current master the classifier is used merely as a hook to get > > > >> into the packet processing within the L2 packet path - that's why you > > > >> see no sessions added. > > > >> > > > >> [PAN]: Cool. Correct me if I am wrong: ingress packet will first > >> check > > > >> against if there is matched session existing > > > >> in fa_node.c and if not, the packet will check against ACL then to > >> decide > > > >> whether to create a new session or drop. > > > > > > > > Exactly, that is the idea! > > > > > > > >> > > > >> > > > >> Is there any 'match then action' alike behavior existing in fa_node? > >> And a > > > >> long standing mystery question for me: what > > > >> does "fa" stand for :p > > > >> > > > > > > > > "FA" is "feature arc" :) When adding the routed path, the feature arc > > > > support appeared a bit earlier, and I was "wow, this is so excellent > > > > and easy", and I thought I might use the same mechanism in L2 as well, > > > > so I just bluntly called everything "fa_*". But then I decided to > > > > leave for L2 data path the old mechanism of (ab)using the L2 > > > > classifier, since it allowed for a while to retain (just in case) the > > > > ability to switch between the old and the new data path. As the time > > > > goes, I suspect there might be a better way to hook in L2 forwarding > > > > path... > > > > > > > >> > > > >> > > > >>> > > > >>> > > > >>> Is there any document/idea could basically explain the relationships > > > >>> between > > > >>> acl/fa_node and vnet_classify? > > > >> > > > >> In a gerrit draft that I am working now on, which aims to provide the > > > >> multicore support, there is text file with the history and the > > > >> description of operation. If you like, I can add you to the reviewers > > > >> list so you can have a look at that - just send me your email that > >> you > > > >> use in gerrit. > > > >> > > > >> [PAN]: Thank you Andrew, I believe this will help a lot. My email is > > > >> dazhangpan at > >> gmail.com<https://lists.fd.io/mailman/listinfo/vpp-dev> > > > >> > > > > > > > > Excellent, I've just added you, it's 6771. As you notice, it's still a > > > > good amount of work to do (lots of clib_warning()s to ensure > > > > everything indeed does what I think it does). Looking forward to your > > > > feedback! :-) > > > > > > > > --a > > > > > > > >> > > > >> > > > >> --a > > > >> > > > >>> > > > >>> > > > >>> Any help will be much appreciated. > > > >>> > > > >>> > > > >>> Best Regards, > > > >>> > > > >>> > > > >>> Pan > > > > _______________________________________________ vpp-dev mailing list vpp-dev@lists.fd.io https://lists.fd.io/mailman/listinfo/vpp-dev
