On 6/30/20 6:43 AM, Eli Britstein wrote: > > On 6/29/2020 9:10 PM, Ilya Maximets wrote: >>>> While calling from the userspace datapath, we always have a match on >>>> dl_type. >>>> This means that it's not possible to hit the 'else' condition. >>>> I'm worried about the usecase described there. It's hard to track changes >>>> in i40e_flow.c. Can someone test this with XL710? >>> Yes, that's true. I think we should have it for consistency with the rest >>> of the code, and also just in case. >>> >>> Regarding the XL710 comment - I think it should not have been merged into >>> OVS in the first place. If it is an issue with XL710, the relevant PMD >>> should handle it, and not OVS. I just kept it in case I miss something >>> here. Do you think it's OK to remove it? >> Thinking about correctness, if dl_type match requested and we're >> matching any L2 packets instead, this does not sound good. In >> this case we might perform incorrect set of actions for a packet >> that should be handled differently. Also, we might end up having >> several flows with the same match and different actions in case the >> only dufference was in dl_type, which is not a good thing too. > This commit addresses exactly this issue, and applies specific ETH matches if > applicable, including dl_type. >> So, from that point of view it's better to remove the 'else' branch >> entirely. Good reasoning should be described in a commit message. > I don't see how that implies removing the else entirely. If there is from > some reason (though won't happen at least currently) no matches on any eth > (src/dst/type), it does make sense to match on generic "ETH". For > completeness I'd prefer to keep the else branch, but maybe drop only the > comment about XL710. What do you think?
AFAIU, rte_flow allows to not specify RTE_FLOW_ITEM_TYPE_ETH. So, I'm not sure why we need to add a match on RTE_FLOW_ITEM_TYPE_ETH if not requested by upper layers. You could remove the current comment, but you will need to add another one to justify adding ETH item while it's not requested. Will HW drop all the packets if we don't have ETH pattern item? That might be a good justification (still HW/PMD specific, though). >> For the 'if' condition: You could do 'masks.dl_type' check first, >> i.e. before checking eth addresses, this way we will save a few cpu >> cycles in most cases. > OK. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
