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

2018-06-18 Thread Finn Christensen 
>-Original Message-
>From: Shahaf Shuler 
>Sent: 18. juni 2018 16:29
>To: Andrew Rybchenko ; Finn Christensen
>; ian.sto...@intel.com
>Cc: d...@openvswitch.org; simon.hor...@netronome.com; f...@redhat.com
>Subject: RE: [ovs-dev] [PATCH v10 4/7] netdev-dpdk: implement flow offload
>with rte flow
>
>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 
>> >
>> > 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 
>> > Signed-off-by: Finn Christensen 
>> > Signed-off-by: Yuanhan Liu 
>> > Signed-off-by: Shahaf Shuler 
>>
>> <...>
>>
>> > +/* VLAN */
>> > +struct rte_flow_item_vlan vlan_spec;
>> > +struct rte_flow_item_vlan vlan_mask;
>> > +memset(_spec, 0, sizeof(vlan_spec));
>> > +memset(_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.

Yes, I agree with this. Thanks Shahaf for correcting this in v11.
Finn

>
>>
>> Andrew.

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2018-06-18 Thread 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 
> >
> > 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 
> > Signed-off-by: Finn Christensen 
> > Signed-off-by: Yuanhan Liu 
> > Signed-off-by: Shahaf Shuler 
> 
> <...>
> 
> > +/* VLAN */
> > +struct rte_flow_item_vlan vlan_spec;
> > +struct rte_flow_item_vlan vlan_mask;
> > +memset(_spec, 0, sizeof(vlan_spec));
> > +memset(_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.

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2018-06-18 Thread Andrew Rybchenko 

On 05/18/2018 12:14 PM, Shahaf Shuler wrote:

From: Finn Christensen 

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 
Signed-off-by: Finn Christensen 
Signed-off-by: Yuanhan Liu 
Signed-off-by: Shahaf Shuler 


<...>


+/* VLAN */
+struct rte_flow_item_vlan vlan_spec;
+struct rte_flow_item_vlan vlan_mask;
+memset(_spec, 0, sizeof(vlan_spec));
+memset(_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);

Andrew.

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2018-06-08 Thread Andrew Rybchenko 

Hi Shahaf,

a couple of nits below

On 05/18/2018 12:14 PM, Shahaf Shuler wrote:

From: Finn Christensen 

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 
Signed-off-by: Finn Christensen 
Signed-off-by: Yuanhan Liu 
Signed-off-by: Shahaf Shuler 
---


<...>


+static int
+netdev_dpdk_flow_put(struct netdev *netdev, struct match *match,
+ struct nlattr *actions, size_t actions_len,
+ const ovs_u128 *ufid, struct offload_info *info,
+ struct dpif_flow_stats *stats OVS_UNUSED) {
+struct rte_flow *rte_flow;
+int ret;
+
+/*
+ * If an old rte_flow exists, it means it's a flow modification.
+ * Here destroy the old rte flow first before adding a new one.
+ */
+rte_flow = ufid_to_rte_flow_find(ufid);
+if (rte_flow) {
+ret = netdev_dpdk_destroy_rte_flow(netdev_dpdk_cast(netdev),
+   ufid, rte_flow);
+if (ret < 0) {
+return ret;
+}
+}
+
+ret = netdev_dpdk_validate_flow(match);
+if (ret < 0) {
+return ret;
+}
+
+return netdev_dpdk_add_rte_flow_offload(netdev, match, actions,
+actions_len, ufid, info); }

Misplaced curly bracket.


+
+static int
+netdev_dpdk_flow_del(struct netdev *netdev, const ovs_u128 *ufid,
+ struct dpif_flow_stats *stats OVS_UNUSED) {
+
+struct rte_flow *rte_flow = ufid_to_rte_flow_find(ufid);
+
+if (!rte_flow) {
+return -1;
+}
+
+return netdev_dpdk_destroy_rte_flow(netdev_dpdk_cast(netdev),
+ufid, rte_flow); }

Misplaced curly bracket.

<...>
The information contained in this message is confidential and is intended for 
the addressee(s) only. If you have received this message in error, please 
notify the sender immediately and delete the message. Unless you are an 
addressee (or authorized to receive for an addressee), you may not use, copy or 
disclose to anyone this message or any information contained in this message. 
The unauthorized use, disclosure, copying or alteration of this message is 
strictly prohibited.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev