[ovs-dev] [PATCH v10 4/7] netdev-dpdk: implement flow offload with rte flow

[Shahaf Shuler]

Monday, June 18, 2018 1:17 PM, Andrew Rybchenko:
> Subject: Re: [ovs-dev] [PATCH v10 4/7] netdev-dpdk: implement flow offload
> with rte flow
> 
> On 05/18/2018 12:14 PM, Shahaf Shuler wrote:
> > From: Finn Christensen <fc at napatech.com>
> >
> > The basic yet the major part of this patch is to translate the "match"
> > to rte flow patterns. And then, we create a rte flow with MARK + RSS
> > actions. Afterwards, all packets match the flow will have the mark id
> > in the mbuf.
> >
> > The reason RSS is needed is, for most NICs, a MARK only action is not
> > allowed. It has to be used together with some other actions, such as
> > QUEUE, RSS, etc. However, QUEUE action can specify one queue only,
> > which may break the rss. Likely, RSS action is currently the best we
> > could now. Thus, RSS action is choosen.
> >
> > For any unsupported flows, such as MPLS, -1 is returned, meaning the
> > flow offload is failed and then skipped.
> >
> > Co-authored-by: Yuanhan Liu <yliu at fridaylinux.org>
> > Signed-off-by: Finn Christensen <fc at napatech.com>
> > Signed-off-by: Yuanhan Liu <yliu at fridaylinux.org>
> > Signed-off-by: Shahaf Shuler <shahafs at mellanox.com>
> 
> <...>
> 
> > +    /* VLAN */
> > +    struct rte_flow_item_vlan vlan_spec;
> > +    struct rte_flow_item_vlan vlan_mask;
> > +    memset(&vlan_spec, 0, sizeof(vlan_spec));
> > +    memset(&vlan_mask, 0, sizeof(vlan_mask));
> > +    if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
> > +        vlan_spec.tci  = match->flow.vlans[0].tci;
> > +        vlan_mask.tci  = match->wc.masks.vlans[0].tci;
> 
> As I understand VLAN_CFI bit is used inside OVS to distinguish no VLAN and
> zero VLAN cases (aka VLAN_TAG_PRESENT). So above two lines should drop
> the VLAN_CFI bit, something like:
> 
>      vlan_spec.tci = match->flow.vlans[0].tci & ~htons(VLAN_CFI);
>      vlan_mask.tci = match->wc.masks.vlans[0].tci & ~htons(VLAN_CFI);

Yes, thanks for spotting that. I guess matching on the CFI bit is not so important from offload perspective, I don't think we will have different actions of those. 
Will be addressed on v11. 

> 
> Andrew.