From: John Fastabend <john.fastabend@gmail.com> Date: 2016-02-03 09:27:51
This extends the setup_tc framework so it can support more than just
the mqprio offload and push other classifiers and qdiscs into the
hardware. The series here targets the u32 classifier and ixgbe
driver. I worked out the u32 classifier because it is protocol
oblivious and aligns with multiple hardware devices I have access
to. I did an initial implementation on ixgbe because (a) I have one
in my box (b) its a stable driver and (c) it is relatively simple
compared to the other devices I have here but still has enough
flexibility to exercise the features of cls_u32.
I intentionally limited the scope of this series to the basic
feature set. Specifically this uses a 'big hammer' feature bit
to do the offload or not. If the bit is set you get offloaded rules
if it is not then rules will not be offloaded. If we can agree on
this patch series there are some more patches on my queue we can
talk about to make the offload decision per rule using flags similar
to how we do l2 mac updates. Additionally the error strategy can
be improved to be hard aborting, log and continue, etc. I think
these are nice to have improvements but shouldn't block this series.
Also by adding get_parse_graph and set_parse_graph attributes as
in my previous flow_api work we can build programmable devices
and programmatically learn when rules can or can not be loaded
into the hardware. Again future work.
Any comments/feedback appreciated.
Thanks,
John
---
John Fastabend (7):
net: rework ndo tc op to consume additional qdisc handle parameter
net: rework setup_tc ndo op to consume general tc operand
net: sched: add cls_u32 offload hooks for netdevs
net: add tc offload feature flag
net: tc: helper functions to query action types
net: ixgbe: add minimal parser details for ixgbe
net: ixgbe: add support for tc_u32 offload
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 8 +
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h | 2
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 2
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 9 +
drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 11 +
drivers/net/ethernet/intel/i40e/i40e_main.c | 10 +
drivers/net/ethernet/intel/ixgbe/ixgbe.h | 3
drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 6 -
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 206 ++++++++++++++++++++++
drivers/net/ethernet/intel/ixgbe/ixgbe_model.h | 112 ++++++++++++
drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 13 +
drivers/net/ethernet/sfc/efx.h | 3
drivers/net/ethernet/sfc/tx.c | 10 +
drivers/net/ethernet/ti/netcp_core.c | 14 +
include/linux/netdev_features.h | 3
include/linux/netdevice.h | 24 ++-
include/net/pkt_cls.h | 33 ++++
include/net/tc_act/tc_gact.h | 16 ++
net/core/ethtool.c | 1
net/sched/cls_u32.c | 73 ++++++++
net/sched/sch_mqprio.c | 8 +
21 files changed, 541 insertions(+), 26 deletions(-)
create mode 100644 drivers/net/ethernet/intel/ixgbe/ixgbe_model.h
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-02-03 09:28:12
The ndo_setup_tc() op was added to support drivers offloading tx
qdiscs however only support for mqprio was ever added. So we
only ever added support for passing the number of traffic classes
to the driver.
This patch generalizes the ndo_setup_tc op so that a handle can
be provided to indicate if the offload is for ingress or egress
or potentially even child qdiscs.
CC: Murali Karicheri <redacted>
CC: Shradha Shah <redacted>
CC: Or Gerlitz <redacted>
CC: Ariel Elior <redacted>
CC: Jeff Kirsher <redacted>
CC: Bruce Allan <redacted>
CC: Jesse Brandeburg <redacted>
CC: Don Skidmore <redacted>
Signed-off-by: John Fastabend <redacted>
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 7 +++++++
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h | 1 +
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 2 +-
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 5 ++++-
drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 10 +++++++++-
drivers/net/ethernet/intel/i40e/i40e_main.c | 9 ++++++++-
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 11 ++++++++++-
drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 12 ++++++++++--
drivers/net/ethernet/sfc/efx.h | 2 +-
drivers/net/ethernet/sfc/tx.c | 5 ++++-
drivers/net/ethernet/ti/netcp_core.c | 5 ++++-
include/linux/netdevice.h | 2 +-
net/sched/sch_mqprio.c | 5 +++--
13 files changed, 63 insertions(+), 13 deletions(-)
@@ -4272,6 +4272,13 @@ int bnx2x_setup_tc(struct net_device *dev, u8 num_tc)return0;}+int__bnx2x_setup_tc(structnet_device*dev,u32handle,u8num_tc)+{+if(handle!=TC_H_ROOT)+return-EINVAL;+returnbnx2x_setup_tc(dev,num_tc);+}+/* called with rtnl_lock */intbnx2x_change_mac_addr(structnet_device*dev,void*p){
@@ -5370,10 +5370,13 @@ static int bnxt_change_mtu(struct net_device *dev, int new_mtu)return0;}-staticintbnxt_setup_tc(structnet_device*dev,u8tc)+staticintbnxt_setup_tc(structnet_device*dev,u32handle,u8tc){structbnxt*bp=netdev_priv(dev);+if(handle!=TC_H_ROOT)+return-EINVAL;+if(tc>bp->max_tc){netdev_err(dev,"too many traffic classes requested: %d Max supported is %d\n",tc,bp->max_tc);
@@ -8200,6 +8200,15 @@ int ixgbe_setup_tc(struct net_device *dev, u8 tc)return0;}+int__ixgbe_setup_tc(structnet_device*dev,u32handle,u8tc)+{+/* Only support egress tc setup for now */+if(handle!=TC_H_ROOT)+return-EINVAL;++returnixgbe_setup_tc(dev,tc);+}+#ifdef CONFIG_PCI_IOVvoidixgbe_sriov_reinit(structixgbe_adapter*adapter){
@@ -1835,13 +1835,16 @@ static u16 netcp_select_queue(struct net_device *dev, struct sk_buff *skb,return0;}-staticintnetcp_setup_tc(structnet_device*dev,u8num_tc)+staticintnetcp_setup_tc(structnet_device*dev,u32handle,u8num_tc){inti;/* setup tc must be called under rtnl lock */ASSERT_RTNL();+if(handle!=TC_H_ROOT)+return-EINVAL;+/* Sanity-check the number of traffic classes requested */if((dev->real_num_tx_queues<=1)||(dev->real_num_tx_queues<num_tc))
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-02-03 09:28:33
This patch updates setup_tc so we can pass additional parameters into
the ndo op in a generic way. To do this we provide structured union
and type flag.
This lets each classifier and qdisc provide its own set of attributes
without having to add new ndo ops or grow the signature of the
callback.
Signed-off-by: John Fastabend <redacted>
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 7 ++++---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h | 3 ++-
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 8 ++++++--
drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 7 ++++---
drivers/net/ethernet/intel/i40e/i40e_main.c | 7 ++++---
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 7 ++++---
drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 7 ++++---
drivers/net/ethernet/sfc/efx.h | 3 ++-
drivers/net/ethernet/sfc/tx.c | 9 ++++++---
drivers/net/ethernet/ti/netcp_core.c | 13 +++++++------
include/linux/netdevice.h | 20 +++++++++++++++++++-
net/sched/sch_mqprio.c | 9 ++++++---
12 files changed, 68 insertions(+), 32 deletions(-)
@@ -4272,11 +4272,12 @@ int bnx2x_setup_tc(struct net_device *dev, u8 num_tc)return0;}-int__bnx2x_setup_tc(structnet_device*dev,u32handle,u8num_tc)+int__bnx2x_setup_tc(structnet_device*dev,u32handle,__be16proto,+structtc_to_netdev*tc){-if(handle!=TC_H_ROOT)+if(handle!=TC_H_ROOT||tc->type!=TC_SETUP_MQPRIO)return-EINVAL;-returnbnx2x_setup_tc(dev,num_tc);+returnbnx2x_setup_tc(dev,tc->tc);}/* called with rtnl_lock */
@@ -5370,13 +5370,17 @@ static int bnxt_change_mtu(struct net_device *dev, int new_mtu)return0;}-staticintbnxt_setup_tc(structnet_device*dev,u32handle,u8tc)+staticintbnxt_setup_tc(structnet_device*dev,u32handle,__be16proto,+structtc_to_netdev*ntc){structbnxt*bp=netdev_priv(dev);+u8tc;-if(handle!=TC_H_ROOT)+if(handle!=TC_H_ROOT||ntc->type!=TC_SETUP_MQPRIO)return-EINVAL;+tc=ntc->tc;+if(tc>bp->max_tc){netdev_err(dev,"too many traffic classes requested: %d Max supported is %d\n",tc,bp->max_tc);
@@ -8200,13 +8200,14 @@ int ixgbe_setup_tc(struct net_device *dev, u8 tc)return0;}-int__ixgbe_setup_tc(structnet_device*dev,u32handle,u8tc)+int__ixgbe_setup_tc(structnet_device*dev,u32handle,__be16proto,+structtc_to_netdev*tc){/* Only support egress tc setup for now */-if(handle!=TC_H_ROOT)+if(handle!=TC_H_ROOT||tc->type!=TC_SETUP_MQPRIO)return-EINVAL;-returnixgbe_setup_tc(dev,tc);+returnixgbe_setup_tc(dev,tc->tc);}#ifdef CONFIG_PCI_IOV
@@ -1835,25 +1835,26 @@ static u16 netcp_select_queue(struct net_device *dev, struct sk_buff *skb,return0;}-staticintnetcp_setup_tc(structnet_device*dev,u32handle,u8num_tc)+staticintnetcp_setup_tc(structnet_device*dev,u32handle,__be16proto,+structtc_to_netdevtc){inti;/* setup tc must be called under rtnl lock */ASSERT_RTNL();-if(handle!=TC_H_ROOT)+if(handle!=TC_H_ROOT||tc->type!=TC_SETUP_MQPRIO)return-EINVAL;/* Sanity-check the number of traffic classes requested */if((dev->real_num_tx_queues<=1)||-(dev->real_num_tx_queues<num_tc))+(dev->real_num_tx_queues<tc->tc))return-EINVAL;/* Configure traffic class to queue mappings */-if(num_tc){-netdev_set_num_tc(dev,num_tc);-for(i=0;i<num_tc;i++)+if(tc->tc){+netdev_set_num_tc(dev,tc->tc);+for(i=0;i<tc->tc;i++)netdev_set_tc_queue(dev,i,1,i);}else{netdev_reset_tc(dev);
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-02-03 09:28:54
This patch allows netdev drivers to consume cls_u32 offloads via
the ndo_setup_tc ndo op.
This works aligns with how network drivers have been doing qdisc
offloads for mqprio.
Signed-off-by: John Fastabend <redacted>
---
include/linux/netdevice.h | 6 +++-
include/net/pkt_cls.h | 33 ++++++++++++++++++++
net/sched/cls_u32.c | 73 ++++++++++++++++++++++++++++++++++++++++++++-
3 files changed, 109 insertions(+), 3 deletions(-)
@@ -778,17 +778,21 @@ static inline bool netdev_phys_item_id_same(struct netdev_phys_item_id *a,typedefu16(*select_queue_fallback_t)(structnet_device*dev,structsk_buff*skb);-/* This structure holds attributes of qdisc and classifiers+/* These structures hold the attributes of qdisc and classifiers*thatarebeingpassedtothenetdevicethroughthesetup_tcop.*/enum{TC_SETUP_MQPRIO,+TC_SETUP_CLSU32,};+structtc_cls_u32_offload;+structtc_to_netdev{unsignedinttype;union{u8tc;+structtc_cls_u32_offload*cls_u32;};};
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-02-03 09:29:15
Its useful to turn off the qdisc offload feature at a per device
level. This gives us a big hammer to enable/disable offloading.
More fine grained control (i.e. per rule) may be supported later.
Signed-off-by: John Fastabend <redacted>
---
include/linux/netdev_features.h | 3 +++
net/core/ethtool.c | 1 +
2 files changed, 4 insertions(+)
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-02-03 09:29:34
This is a helper function drivers can use to learn if the
action type is a drop action.
Signed-off-by: John Fastabend <redacted>
---
include/net/tc_act/tc_gact.h | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-02-03 09:29:55
This adds an ixgbe data structure that is used to determine what
headers:fields can be matched and in what order they are supported.
For hardware devices this can be a bit tricky because typically
only pre-programmed (firmware, ucode, rtl) parse graphs will be
supported and we don't yet have an interface to change these from
the OS. So its sort of a you get whatever your friendly vendor
provides affair at the moment.
In the future we can add the get routines and set routines to
update this data structure. One interesting thing to note here
is the data structure here identifies ethernet, ip, and tcp
fields without having to hardcode them as enumerations or use
other identifiers.
Signed-off-by: John Fastabend <redacted>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_model.h | 112 ++++++++++++++++++++++++
1 file changed, 112 insertions(+)
create mode 100644 drivers/net/ethernet/intel/ixgbe/ixgbe_model.h
@@ -0,0 +1,112 @@+/*******************************************************************************+*+*Intel10GigabitPCIExpressLinuxdrive+*Copyright(c)2013-2015IntelCorporation.+*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodifyit+*underthetermsandconditionsoftheGNUGeneralPublicLicense,+*version2,aspublishedbytheFreeSoftwareFoundation.+*+*Thisprogramisdistributedinthehopeitwillbeuseful,butWITHOUT+*ANYWARRANTY;withouteventheimpliedwarrantyofMERCHANTABILITYor+*FITNESSFORAPARTICULARPURPOSE.SeetheGNUGeneralPublicLicensefor+*moredetails.+*+*YoushouldhavereceivedacopyoftheGNUGeneralPublicLicensealong+*withthisprogram.Ifnot,see<http://www.gnu.org/licenses/>.+*+*ThefullGNUGeneralPublicLicenseisincludedinthisdistributionin+*thefilecalled"COPYING".+*+*ContactInformation:+*e1000-develMailingList<e1000-devel@lists.sourceforge.net>+*IntelCorporation,5200N.E.ElamYoungParkway,Hillsboro,OR97124-6497+*+******************************************************************************/++#ifndef _IXGBE_MODEL_H_+#define _IXGBE_MODEL_H_++#include"ixgbe.h"+#include"ixgbe_type.h"++structixgbe_mat_field{+unsignedintoff;+unsignedintmask;+int(*val)(structixgbe_fdir_filter*input,+unionixgbe_atr_input*mask,+__u32val,__u32m);+unsignedinttype;+};++staticinlineintixgbe_mat_prgm_sip(structixgbe_fdir_filter*input,+unionixgbe_atr_input*mask,+__u32val,__u32m)+{+input->filter.formatted.src_ip[0]=val;+mask->formatted.src_ip[0]=m;+return0;+}++staticinlineintixgbe_mat_prgm_dip(structixgbe_fdir_filter*input,+unionixgbe_atr_input*mask,+__u32val,__u32m)+{+input->filter.formatted.dst_ip[0]=val;+mask->formatted.dst_ip[0]=m;+return0;+}++staticstructixgbe_mat_fieldixgbe_ipv4_fields[]={+{.off=12,.mask=-1,.val=ixgbe_mat_prgm_sip,+.type=IXGBE_ATR_FLOW_TYPE_IPV4},+{.off=16,.mask=-1,.val=ixgbe_mat_prgm_dip,+.type=IXGBE_ATR_FLOW_TYPE_IPV4},+{.val=NULL}/* terminal node */+};++staticinlineintixgbe_mat_prgm_sport(structixgbe_fdir_filter*input,+unionixgbe_atr_input*mask,+__u32val,__u32m)+{+input->filter.formatted.src_port=val&0xffff;+mask->formatted.src_port=m&0xffff;+return0;+};++staticinlineintixgbe_mat_prgm_dport(structixgbe_fdir_filter*input,+unionixgbe_atr_input*mask,+__u32val,__u32m)+{+input->filter.formatted.dst_port=val&0xffff;+mask->formatted.dst_port=m&0xffff;+return0;+};++staticstructixgbe_mat_fieldixgbe_tcp_fields[]={+{.off=0,.mask=0xffff,.val=ixgbe_mat_prgm_sport,+.type=IXGBE_ATR_FLOW_TYPE_TCPV4},+{.off=2,.mask=0xffff,.val=ixgbe_mat_prgm_dport,+.type=IXGBE_ATR_FLOW_TYPE_TCPV4},+{.val=NULL}/* terminal node */+};++structixgbe_nexthdr{+/* offset, shift, and mask of position to next header */+unsignedinto;+__u32s;+__u32m;+/* match criteria to make this jump*/+unsignedintoff;+__u32val;+__u32mask;+/* location of jump to make */+structixgbe_mat_field*jump;+};++staticstructixgbe_nexthdrixgbe_ipv4_jumps[]={+{.o=0,.s=6,.m=0xf,+.off=8,.val=0x600,.mask=0xff00,.jump=ixgbe_tcp_fields},+{.jump=NULL}/* terminal node */+};+#endif /* _IXGBE_MODEL_H_ */
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-02-03 09:30:17
This adds initial support for offloading the u32 tc classifier. This
initial implementation only implements a few base matches and actions
to illustrate the use of the infrastructure patches.
However it is an interesting subset because it handles the u32 next
hdr logic to correctly map tcp packets from ip headers using the ihl
and protocol fields. After this is accepted we can extend the match
and action fields easily by updating the model header file.
Also only the drop action is supported initially.
Here is a short test script,
#tc qdisc add dev eth4 ingress
#tc filter add dev eth4 parent ffff: protocol ip \
u32 ht 800: order 1 \
match ip dst 15.0.0.1/32 match ip src 15.0.0.2/32 action drop
<-- hardware has dst/src ip match rule installed -->
#tc filter del dev eth4 parent ffff: prio 49152
#tc filter add dev eth4 parent ffff: protocol ip prio 99 \
handle 1: u32 divisor 1
#tc filter add dev eth4 protocol ip parent ffff: prio 99 \
u32 ht 800: order 1 link 1: \
offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6 ff
#tc filter add dev eth4 parent ffff: protocol ip \
u32 ht 1: order 3 match tcp src 23 ffff action drop
<-- hardware has tcp src port rule installed -->
#tc qdisc del dev eth4 parent ffff:
<-- hardware cleaned up -->
Signed-off-by: John Fastabend <redacted>
---
drivers/net/ethernet/intel/ixgbe/ixgbe.h | 3
drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 6 -
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 196 ++++++++++++++++++++++
3 files changed, 198 insertions(+), 7 deletions(-)
@@ -8200,10 +8201,197 @@ int ixgbe_setup_tc(struct net_device *dev, u8 tc)return0;}+#include<net/tc_act/tc_gact.h>+#include"ixgbe_model.h"+staticintixgbe_delete_clsu32(structixgbe_adapter*adapter,+structtc_cls_u32_offload*cls)+{+interr;++spin_lock(&adapter->fdir_perfect_lock);+err=ixgbe_update_ethtool_fdir_entry(adapter,NULL,cls->knode.handle);+spin_unlock(&adapter->fdir_perfect_lock);+returnerr;+}++#define IXGBE_MAX_LINK_HANDLE 10+staticstructixgbe_mat_field*+ixgbe_jump_tables[IXGBE_MAX_LINK_HANDLE]={ixgbe_ipv4_fields,};++staticintixgbe_configure_clsu32(structixgbe_adapter*adapter,+__be16protocol,+structtc_cls_u32_offload*cls)+{+u32loc=cls->knode.handle&0xfffff;+structixgbe_hw*hw=&adapter->hw;+structixgbe_mat_field*field_ptr;+structixgbe_fdir_filter*input;+unionixgbe_atr_inputmask;+#ifdef CONFIG_NET_CLS_ACT+conststructtc_action*a;+#endif+inti,err=0;+u8queue;+u32handle;++memset(&mask,0,sizeof(unionixgbe_atr_input));+handle=cls->knode.handle;++/* At the moment cls_u32 jumps to transport layer and skips past+*L2headers.ThecanonicalmethodtomatchL2framesistouse+*negativevalues.Howeverthisiserrorproneatbestbutreally+*justbrokenbecausethereisnowayto"know"whatsortofhdr+*isinfrontofthetransportlayer.Fixcls_u32tosupportL2+*headerswhenneeded.+*/+if(protocol!=htons(ETH_P_IP))+return-EINVAL;++if(cls->knode.link_handle||+cls->knode.link_handle>=IXGBE_MAX_LINK_HANDLE){+structixgbe_nexthdr*nexthdr=ixgbe_ipv4_jumps;+u32uhtid=TC_U32_USERHTID(cls->knode.link_handle);++for(i=0;nexthdr[i].jump;i++){+if(nexthdr->o!=cls->knode.sel->offoff||+nexthdr->s!=cls->knode.sel->offshift||+nexthdr->m!=cls->knode.sel->offmask||+/* do not support multiple key jumps its just mad */+cls->knode.sel->nkeys>1)+return-EINVAL;++if(nexthdr->off!=cls->knode.sel->keys[0].off||+nexthdr->val!=cls->knode.sel->keys[0].val||+nexthdr->mask!=cls->knode.sel->keys[0].mask)+return-EINVAL;++if(uhtid>=IXGBE_MAX_LINK_HANDLE)+return-EINVAL;++ixgbe_jump_tables[uhtid]=nexthdr->jump;+}+return0;+}++if(loc>=((1024<<adapter->fdir_pballoc)-2)){+e_err(drv,"Location out of range\n");+return-EINVAL;+}++/* cls u32 is a graph starting at root node 0x800. The driver tracks+*linksandalsothefieldsusedtoadvancetheparseracrosseach+*link(e.g.nexthdr/eatparametersfrom'tc').Thiswaywecanmap+*theu32graphontothehardwareparsegraphdenotedinixgbe_model.h+*Toaddsupportfornewnodesupdateixgbe_model.hparsestructures+*thisfunction_should_begenerictrynottohardcodevalueshere.+*/+if(TC_U32_USERHTID(handle)==0x800){+field_ptr=ixgbe_jump_tables[0];+}else{+if(TC_U32_USERHTID(handle)>=ARRAY_SIZE(ixgbe_jump_tables))+return-EINVAL;++field_ptr=ixgbe_jump_tables[TC_U32_USERHTID(handle)];+}++if(!field_ptr)+return-EINVAL;++input=kzalloc(sizeof(*input),GFP_KERNEL);+if(!input)+return-ENOMEM;++for(i=0;i<cls->knode.sel->nkeys;i++){+intoff=cls->knode.sel->keys[i].off;+__be32val=cls->knode.sel->keys[i].val;+__be32m=cls->knode.sel->keys[i].mask;+boolfound_entry=false;+intj;++for(j=0;field_ptr[j].val;j++){+if(field_ptr[j].off==off&&+field_ptr[j].mask==m){+field_ptr[j].val(input,&mask,val,m);+input->filter.formatted.flow_type|=+field_ptr[j].type;+found_entry=true;+break;+}+}++if(!found_entry)+gotoerr_out;+}++mask.formatted.flow_type=IXGBE_ATR_L4TYPE_IPV6_MASK|+IXGBE_ATR_L4TYPE_MASK;++if(input->filter.formatted.flow_type==IXGBE_ATR_FLOW_TYPE_IPV4)+mask.formatted.flow_type&=IXGBE_ATR_L4TYPE_IPV6_MASK;++#ifdef CONFIG_NET_CLS_ACT+if(list_empty(&cls->knode.exts->actions))+gotoerr_out;++list_for_each_entry(a,&cls->knode.exts->actions,list){+if(!is_tcf_gact_dropped(a))+gotoerr_out;+}+#endif++input->action=IXGBE_FDIR_DROP_QUEUE;+queue=IXGBE_FDIR_DROP_QUEUE;+input->sw_idx=loc;++spin_lock(&adapter->fdir_perfect_lock);++if(hlist_empty(&adapter->fdir_filter_list)){+memcpy(&adapter->fdir_mask,&mask,sizeof(mask));+err=ixgbe_fdir_set_input_mask_82599(hw,&mask);+if(err)+gotoerr_out_w_lock;+}elseif(memcmp(&adapter->fdir_mask,&mask,sizeof(mask))){+err=-EINVAL;+gotoerr_out_w_lock;+}++ixgbe_atr_compute_perfect_hash_82599(&input->filter,&mask);+err=ixgbe_fdir_write_perfect_filter_82599(hw,&input->filter,+input->sw_idx,queue);+if(!err)+ixgbe_update_ethtool_fdir_entry(adapter,input,input->sw_idx);+spin_unlock(&adapter->fdir_perfect_lock);++returnerr;+err_out_w_lock:+spin_unlock(&adapter->fdir_perfect_lock);+err_out:+kfree(input);+return-EINVAL;+}+int__ixgbe_setup_tc(structnet_device*dev,u32handle,__be16proto,structtc_to_netdev*tc){-/* Only support egress tc setup for now */+structixgbe_adapter*adapter=netdev_priv(dev);++if(TC_H_MAJ(handle)==TC_H_MAJ(TC_H_INGRESS)&&+tc->type==TC_SETUP_CLSU32){+if(!(dev->hw_features&NETIF_F_HW_TC))+return-EINVAL;++switch(tc->cls_u32->command){+caseTC_CLSU32_NEW_KNODE:+caseTC_CLSU32_REPLACE_KNODE:+returnixgbe_configure_clsu32(adapter,+proto,tc->cls_u32);+caseTC_CLSU32_DELETE_KNODE:+returnixgbe_delete_clsu32(adapter,tc->cls_u32);+default:+return-EINVAL;+}+}+if(handle!=TC_H_ROOT||tc->type!=TC_SETUP_MQPRIO)return-EINVAL;
@@ -8277,6 +8465,7 @@ static int ixgbe_set_features(struct net_device *netdev,*/switch(features&NETIF_F_NTUPLE){caseNETIF_F_NTUPLE:+caseNETIF_F_HW_TC:/* turn off ATR, enable perfect filters and reset */if(!(adapter->flags&IXGBE_FLAG_FDIR_PERFECT_CAPABLE))need_reset=true;
From: kbuild test robot <hidden> Date: 2016-02-03 09:59:49
Hi John,
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/John-Fastabend/tc-offload-for-cls_u32-on-ixgbe/20160203-173342
config: x86_64-randconfig-x016-201605 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/net/ethernet/intel/fm10k/fm10k_netdev.c: In function '__fm10k_setup_tc':
quoted
drivers/net/ethernet/intel/fm10k/fm10k_netdev.c:1209:16: error: 'TC_H_ROOT' undeclared (first use in this function)
if (handle != TC_H_ROOT)
^
drivers/net/ethernet/intel/fm10k/fm10k_netdev.c:1209:16: note: each undeclared identifier is reported only once for each function it appears in
vim +/TC_H_ROOT +1209 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
1203
1204 return err;
1205 }
1206
1207 static int __fm10k_setup_tc(struct net_device *dev, u32 handle, u8 tc)
1208 {
1209 if (handle != TC_H_ROOT)
1210 return -EINVAL;
1211
1212 return fm10k_setup_tc(dev, tc);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
From: kbuild test robot <hidden> Date: 2016-02-03 10:01:10
Hi John,
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/John-Fastabend/tc-offload-for-cls_u32-on-ixgbe/20160203-173342
config: x86_64-randconfig-x005-201605 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
In file included from include/uapi/linux/stddef.h:1:0,
from include/linux/stddef.h:4,
from include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from drivers/net/ethernet/intel/fm10k/fm10k.h:24,
from drivers/net/ethernet/intel/fm10k/fm10k_netdev.c:21:
drivers/net/ethernet/intel/fm10k/fm10k_netdev.c: In function '__fm10k_setup_tc':
drivers/net/ethernet/intel/fm10k/fm10k_netdev.c:1209:16: error: 'TC_H_ROOT' undeclared (first use in this function)
if (handle != TC_H_ROOT)
^
include/linux/compiler.h:147:28: note: in definition of macro '__trace_if'
if (__builtin_constant_p((cond)) ? !!(cond) : \
^
quoted
drivers/net/ethernet/intel/fm10k/fm10k_netdev.c:1209:2: note: in expansion of macro 'if'
if (handle != TC_H_ROOT)
^
drivers/net/ethernet/intel/fm10k/fm10k_netdev.c:1209:16: note: each undeclared identifier is reported only once for each function it appears in
if (handle != TC_H_ROOT)
^
include/linux/compiler.h:147:28: note: in definition of macro '__trace_if'
if (__builtin_constant_p((cond)) ? !!(cond) : \
^
quoted
drivers/net/ethernet/intel/fm10k/fm10k_netdev.c:1209:2: note: in expansion of macro 'if'
if (handle != TC_H_ROOT)
^
vim +/if +1209 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
1193 /* flag to indicate SWPRI has yet to be updated */
1194 interface->flags |= FM10K_FLAG_SWPRI_CONFIG;
1195
1196 return 0;
1197 err_open:
1198 fm10k_mbx_free_irq(interface);
1199 err_mbx_irq:
1200 fm10k_clear_queueing_scheme(interface);
1201 err_queueing_scheme:
1202 netif_device_detach(dev);
1203
1204 return err;
1205 }
1206
1207 static int __fm10k_setup_tc(struct net_device *dev, u32 handle, u8 tc)
1208 {
1209 if (handle != TC_H_ROOT)
1210 return -EINVAL;
1211
1212 return fm10k_setup_tc(dev, tc);
1213 }
1214
1215 static int fm10k_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
1216 {
1217 switch (cmd) {
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
From: Amir Vadai" <hidden> Date: 2016-02-03 10:04:21
On Wed, Feb 03, 2016 at 01:29:59AM -0800, John Fastabend wrote:
This adds initial support for offloading the u32 tc classifier. This
initial implementation only implements a few base matches and actions
to illustrate the use of the infrastructure patches.
However it is an interesting subset because it handles the u32 next
hdr logic to correctly map tcp packets from ip headers using the ihl
and protocol fields. After this is accepted we can extend the match
and action fields easily by updating the model header file.
Also only the drop action is supported initially.
Here is a short test script,
#tc qdisc add dev eth4 ingress
#tc filter add dev eth4 parent ffff: protocol ip \
u32 ht 800: order 1 \
match ip dst 15.0.0.1/32 match ip src 15.0.0.2/32 action drop
<-- hardware has dst/src ip match rule installed -->
#tc filter del dev eth4 parent ffff: prio 49152
#tc filter add dev eth4 parent ffff: protocol ip prio 99 \
handle 1: u32 divisor 1
#tc filter add dev eth4 protocol ip parent ffff: prio 99 \
u32 ht 800: order 1 link 1: \
offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6 ff
#tc filter add dev eth4 parent ffff: protocol ip \
u32 ht 1: order 3 match tcp src 23 ffff action drop
<-- hardware has tcp src port rule installed -->
#tc qdisc del dev eth4 parent ffff:
<-- hardware cleaned up -->
Signed-off-by: John Fastabend <redacted>
---
drivers/net/ethernet/intel/ixgbe/ixgbe.h | 3
drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 6 -
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 196 ++++++++++++++++++++++
3 files changed, 198 insertions(+), 7 deletions(-)
What are you doing w.r.t priorities? Are the filters processed by the
order of the priorities?
[...]
From: Amir Vadai" <hidden> Date: 2016-02-03 10:08:12
On Wed, Feb 03, 2016 at 01:27:32AM -0800, John Fastabend wrote:
This extends the setup_tc framework so it can support more than just
the mqprio offload and push other classifiers and qdiscs into the
hardware. The series here targets the u32 classifier and ixgbe
driver. I worked out the u32 classifier because it is protocol
oblivious and aligns with multiple hardware devices I have access
to. I did an initial implementation on ixgbe because (a) I have one
in my box (b) its a stable driver and (c) it is relatively simple
compared to the other devices I have here but still has enough
flexibility to exercise the features of cls_u32.
I intentionally limited the scope of this series to the basic
feature set. Specifically this uses a 'big hammer' feature bit
to do the offload or not. If the bit is set you get offloaded rules
if it is not then rules will not be offloaded. If we can agree on
this patch series there are some more patches on my queue we can
talk about to make the offload decision per rule using flags similar
to how we do l2 mac updates. Additionally the error strategy can
be improved to be hard aborting, log and continue, etc. I think
these are nice to have improvements but shouldn't block this series.
Also by adding get_parse_graph and set_parse_graph attributes as
in my previous flow_api work we can build programmable devices
and programmatically learn when rules can or can not be loaded
into the hardware. Again future work.
Any comments/feedback appreciated.
Thanks,
John
---
John Fastabend (7):
net: rework ndo tc op to consume additional qdisc handle parameter
net: rework setup_tc ndo op to consume general tc operand
net: sched: add cls_u32 offload hooks for netdevs
net: add tc offload feature flag
net: tc: helper functions to query action types
net: ixgbe: add minimal parser details for ixgbe
net: ixgbe: add support for tc_u32 offload
Hi John,
Nice work :)
I will add mlx5 support, and see if can live with u32. If not - will
add flower support too.
Amir
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-02-03 10:21:44
On 16-02-03 02:11 AM, Amir Vadai" wrote:
On Wed, Feb 03, 2016 at 01:27:32AM -0800, John Fastabend wrote:
quoted
This extends the setup_tc framework so it can support more than just
the mqprio offload and push other classifiers and qdiscs into the
hardware. The series here targets the u32 classifier and ixgbe
driver. I worked out the u32 classifier because it is protocol
oblivious and aligns with multiple hardware devices I have access
to. I did an initial implementation on ixgbe because (a) I have one
in my box (b) its a stable driver and (c) it is relatively simple
compared to the other devices I have here but still has enough
flexibility to exercise the features of cls_u32.
I intentionally limited the scope of this series to the basic
feature set. Specifically this uses a 'big hammer' feature bit
to do the offload or not. If the bit is set you get offloaded rules
if it is not then rules will not be offloaded. If we can agree on
this patch series there are some more patches on my queue we can
talk about to make the offload decision per rule using flags similar
to how we do l2 mac updates. Additionally the error strategy can
be improved to be hard aborting, log and continue, etc. I think
these are nice to have improvements but shouldn't block this series.
Also by adding get_parse_graph and set_parse_graph attributes as
in my previous flow_api work we can build programmable devices
and programmatically learn when rules can or can not be loaded
into the hardware. Again future work.
Any comments/feedback appreciated.
Thanks,
John
---
John Fastabend (7):
net: rework ndo tc op to consume additional qdisc handle parameter
net: rework setup_tc ndo op to consume general tc operand
net: sched: add cls_u32 offload hooks for netdevs
net: add tc offload feature flag
net: tc: helper functions to query action types
net: ixgbe: add minimal parser details for ixgbe
net: ixgbe: add support for tc_u32 offload
Hi John,
Nice work :)
Thanks, we will need at least a v2 to fixup some build errors
with various compile flags caught by build_bot and missed by me.
I will add mlx5 support, and see if can live with u32. If not - will
add flower support too.
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-02-03 10:26:41
On 16-02-03 02:07 AM, Amir Vadai" wrote:
On Wed, Feb 03, 2016 at 01:29:59AM -0800, John Fastabend wrote:
quoted
This adds initial support for offloading the u32 tc classifier. This
initial implementation only implements a few base matches and actions
to illustrate the use of the infrastructure patches.
However it is an interesting subset because it handles the u32 next
hdr logic to correctly map tcp packets from ip headers using the ihl
and protocol fields. After this is accepted we can extend the match
and action fields easily by updating the model header file.
Also only the drop action is supported initially.
Here is a short test script,
#tc qdisc add dev eth4 ingress
#tc filter add dev eth4 parent ffff: protocol ip \
u32 ht 800: order 1 \
match ip dst 15.0.0.1/32 match ip src 15.0.0.2/32 action drop
<-- hardware has dst/src ip match rule installed -->
#tc filter del dev eth4 parent ffff: prio 49152
#tc filter add dev eth4 parent ffff: protocol ip prio 99 \
handle 1: u32 divisor 1
#tc filter add dev eth4 protocol ip parent ffff: prio 99 \
u32 ht 800: order 1 link 1: \
offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6 ff
#tc filter add dev eth4 parent ffff: protocol ip \
u32 ht 1: order 3 match tcp src 23 ffff action drop
<-- hardware has tcp src port rule installed -->
#tc qdisc del dev eth4 parent ffff:
<-- hardware cleaned up -->
Signed-off-by: John Fastabend <redacted>
---
drivers/net/ethernet/intel/ixgbe/ixgbe.h | 3
drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 6 -
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 196 ++++++++++++++++++++++
3 files changed, 198 insertions(+), 7 deletions(-)
What are you doing w.r.t priorities? Are the filters processed by the
order of the priorities?
The rules are put in order by the handles which is populated in
my command above such that 'ht 1: order 3' gives handle 1::3 and
'ht 800: order 1' gives 800::1. Take a look at this block in cls_u32
if (err == 0) {
struct tc_u_knode __rcu **ins;
struct tc_u_knode *pins;
ins = &ht->ht[TC_U32_HASH(handle)];
for (pins = rtnl_dereference(*ins); pins;
ins = &pins->next, pins = rtnl_dereference(*ins))
if (TC_U32_NODE(handle) < TC_U32_NODE(pins->handle))
break;
RCU_INIT_POINTER(n->next, pins);
rcu_assign_pointer(*ins, n);
u32_replace_hw_knode(tp, n);
*arg = (unsigned long)n;
return 0;
If you leave ht and order off the tc cli I believe 'tc' just
picks some semi-arbitrary ones for you. I've been in the habit
of always specifying them even for software filters.
[...]
From: Or Gerlitz <hidden> Date: 2016-02-03 10:31:55
On 2/3/2016 12:21 PM, John Fastabend wrote:
Thanks, we will need at least a v2 to fixup some build errors
with various compile flags caught by build_bot and missed by me.
Hi John,
You didn't mark that as RFC... but we said this direction/approach yet
to be talked @ netdev next-week, so.. can you clarify?
I suggest not to rush and asking pulling this, lets have the tc workshop
beforehand...
Please add to v2 listing of changes from V0/V1 to assist with the review.
thanks,
Or.
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2016-02-03 12:21:32
On 16-02-03 05:31 AM, Or Gerlitz wrote:
On 2/3/2016 12:21 PM, John Fastabend wrote:
quoted
Thanks, we will need at least a v2 to fixup some build errors
with various compile flags caught by build_bot and missed by me.
Hi John,
You didn't mark that as RFC... but we said this direction/approach yet
to be talked @ netdev next-week, so.. can you clarify?
I suggest not to rush and asking pulling this, lets have the tc workshop
beforehand...
Yes, the tc workshop is a good place for this.
I think we can spill some of it into the switchdev workshop (which is a
nice flow since that happens later).
Some comments:
1) "priorities" for filters and some form of "index" for actions is
is needed. I think index (which tends to be a 32 bit value is what
Amir's patches refered to as "cookie" - or at least some hardware
can be used to query the action with). Priorities maybe implicit in
the order in which they are added. And the idea of appending vs
exclusivity vs replace (which netlink already supports)
is important to worry about (TCAMS tend to assume an append mode
for example).
2) I like the u32 approach where it makes sense; but sometimes it
doesnt make sense from a usability pov. I work with some ASICs
that have 10 tuples that are fixed. Yes, a user can describe a policy
with u32 but flower would be more usable say with flower (both
programmatic and cli)
3) The concept of "hook address" is important to be able to express.
Amir's patches seemed to miss that (and John brought it up in an
email). It could be as simple as ifindex + hookid. With ifindex of
0 meaning all ports and maybe hookid of 0 meaning all hooks.
Hook semantics are as mentioned by John (as it stands right now
in/egress)
4) Why are we forsaking switchdev John?
This is certainly re-usable beyond NICs and SRIOV.
5)What happened to being both able to hardware and/or software?
Anyways, I think Seville would be a blast! Come one, come all.
cheers,
jamal
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2016-02-03 12:46:04
On 16-02-03 05:26 AM, John Fastabend wrote:
On 16-02-03 02:07 AM, Amir Vadai" wrote:
quoted
On Wed, Feb 03, 2016 at 01:29:59AM -0800, John Fastabend wrote:
quoted
This adds initial support for offloading the u32 tc classifier. This
initial implementation only implements a few base matches and actions
to illustrate the use of the infrastructure patches.
However it is an interesting subset because it handles the u32 next
hdr logic to correctly map tcp packets from ip headers using the ihl
and protocol fields. After this is accepted we can extend the match
and action fields easily by updating the model header file.
Also only the drop action is supported initially.
Here is a short test script,
#tc qdisc add dev eth4 ingress
#tc filter add dev eth4 parent ffff: protocol ip \
u32 ht 800: order 1 \
match ip dst 15.0.0.1/32 match ip src 15.0.0.2/32 action drop
<-- hardware has dst/src ip match rule installed -->
#tc filter del dev eth4 parent ffff: prio 49152
#tc filter add dev eth4 parent ffff: protocol ip prio 99 \
handle 1: u32 divisor 1
#tc filter add dev eth4 protocol ip parent ffff: prio 99 \
u32 ht 800: order 1 link 1: \
offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6 ff
#tc filter add dev eth4 parent ffff: protocol ip \
u32 ht 1: order 3 match tcp src 23 ffff action drop
<-- hardware has tcp src port rule installed -->
#tc qdisc del dev eth4 parent ffff:
<-- hardware cleaned up -->
Signed-off-by: John Fastabend <redacted>
---
drivers/net/ethernet/intel/ixgbe/ixgbe.h | 3
drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 6 -
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 196 ++++++++++++++++++++++
3 files changed, 198 insertions(+), 7 deletions(-)
What are you doing w.r.t priorities? Are the filters processed by the
order of the priorities?
The rules are put in order by the handles which is populated in
my command above such that 'ht 1: order 3' gives handle 1::3 and
'ht 800: order 1' gives 800::1. Take a look at this block in cls_u32
if (err == 0) {
struct tc_u_knode __rcu **ins;
struct tc_u_knode *pins;
ins = &ht->ht[TC_U32_HASH(handle)];
for (pins = rtnl_dereference(*ins); pins;
ins = &pins->next, pins = rtnl_dereference(*ins))
if (TC_U32_NODE(handle) < TC_U32_NODE(pins->handle))
break;
RCU_INIT_POINTER(n->next, pins);
rcu_assign_pointer(*ins, n);
u32_replace_hw_knode(tp, n);
*arg = (unsigned long)n;
return 0;
If you leave ht and order off the tc cli I believe 'tc' just
picks some semi-arbitrary ones for you. I've been in the habit
of always specifying them even for software filters.
The default table id is essentially 0x800. Default bucket is 0.
"order" essentially is the filter id. And given you can link tables
(Nice work John!); essentially the ht:bucket:nodeid is an "address" to
a specific filter on a specific table and when makes sense a specific
hash bucket. Some other way to look at it is as a way to construct
a mapping to a TCAM key.
What John is doing is essentially taking the nodeid and trying to use
it as a priority. In otherwise the abstraction is reduced to a linked
list in which the ordering is how the list is traversed.
It may work in this case, but i am for being able to explicitly specify
priorities.
cheers,
jamal
From: "Fastabend, John R" <john.fastabend@gmail.com> Date: 2016-02-03 18:49:08
On 2/3/2016 4:21 AM, Jamal Hadi Salim wrote:
On 16-02-03 05:31 AM, Or Gerlitz wrote:
quoted
On 2/3/2016 12:21 PM, John Fastabend wrote:
quoted
Thanks, we will need at least a v2 to fixup some build errors
with various compile flags caught by build_bot and missed by me.
Hi John,
You didn't mark that as RFC... but we said this direction/approach yet
to be talked @ netdev next-week, so.. can you clarify?
Yeah I think this set of patches is ready and it really is what we've
been talking about doing for two or three conferences now. Also I don't
know where "we" decided to talk about this @ netdev or how that became
a prereq for patches. I think this is the correct approach and I am not
seeing any contentious pieces here so why not consider it for inclusion.
Do you have some issue with the approach? I don't recall any when we
talked about this last time.
quoted
I suggest not to rush and asking pulling this, lets have the tc workshop
beforehand...
rush? we've been talking about it for a year+
Yes, the tc workshop is a good place for this.
I think we can spill some of it into the switchdev workshop (which is a
nice flow since that happens later).
Sure but this patch is really the basic stuff we should move on to
some of the more interesting pieces. I have ~30 or so patches behind
this that do the fun stuff like resource allocation, capababilities,
support for the divisor > 1, some new actions, etc.
Some comments:
1) "priorities" for filters and some form of "index" for actions is
is needed. I think index (which tends to be a 32 bit value is what
Amir's patches refered to as "cookie" - or at least some hardware
can be used to query the action with). Priorities maybe implicit in
the order in which they are added. And th idea of appending vs
exclusivity vs replace (which netlink already supports)
is important to worry about (TCAMS tend to assume an append mode
for example).
The code denotes add/del/replace already. I'm not sure why a TCAM
would assume an append mode but OK maybe that is some API you have
the APIs I use don't have these semantics.
For this series using cls_u32 the handle gives you everything you need
to put entries in the right table and row. Namely the ht # and order #
from 'tc'. Take a look at u32_change and u32_classify its the handle
that places the filter into the list and the handle that is matched in
classify. We should place the filters in the hardware in the same order
that is used by u32_change.
Also ran a few tests and can't see how priority works in u32 maybe you
can shed some light but as best I can tell it doesn't have any effect
on rule execution.
2) I like the u32 approach where it makes sense; but sometimes it
doesnt make sense from a usability pov. I work with some ASICs
that have 10 tuples that are fixed. Yes, a user can describe a policy
with u32 but flower would be more usable say with flower (both
programmatic and cli)
Sure so create a set of offload hooks for flower we don't need only
one hardware classifier any more than we would like a single software
classifiers. I'll send out my flower patches when I get to a real system
I'm on a corporate laptop at the moment.
3) The concept of "hook address" is important to be able to express.
Amir's patches seemed to miss that (and John brought it up in an
email). It could be as simple as ifindex + hookid. With ifindex of
0 meaning all ports and maybe hookid of 0 meaning all hooks.
Hook semantics are as mentioned by John (as it stands right now
in/egress)
Again I'm trying to faithfully implement what we have in software
and load that into the hardware. The handle today gives ingress/egres
hook. If you want an all ports hook we should add it to 'tc' software
first and then push that to the hardware not create magic hardware
bits. See I've drank the cool aid software first than hardware.
4) Why are we forsaking switchdev John?
This is certainly re-usable beyond NICs and SRIOV.
Sure and switchdev can use it just like they use fdb_add and friends.
I just don't want to require switchdev infrastructure on things that
really are not switches. I think Amir indicated he would take a try
at the switchdev integration. If not I'm willing to do it but it
doesn't block this series in any way imo.
5)What happened to being both able to hardware and/or software?
Follow up patch once we get the basic infrastructure in place with
the big feature flag bit. I have a patch I'm testing for this now
but again I want to move in logical and somewhat minimal sets.
Anyways, I think Seville would be a blast! Come one, come all.
I'll be there but lets be sure to follow up with this online I
know folks are following this who wont be at Seville and I don't
see any reason to block these patches and stop the thread for a
week or more.
From: "Fastabend, John R" <john.fastabend@gmail.com> Date: 2016-02-03 19:03:06
On 2/3/2016 4:46 AM, Jamal Hadi Salim wrote:
[...]
quoted
quoted
quoted
What are you doing w.r.t priorities? Are the filters processed by the
order of the priorities?
In the same order as software processes filters. I tried to faithfully
translate the u32 classify and u32_change loops into hardware. If I
missed some case its a bug and I'll fix it. My reading and experiments
of u32 indicate the ht:bucket:node build a handle and this is how we
order rules.
When I test this I create a veth pair and create the same rule set on
the veth pair as my hardware netdev. Then inject a skb into both
the veth and hardware netdev if you get different results its a bug.
quoted
The rules are put in order by the handles which is populated in
my command above such that 'ht 1: order 3' gives handle 1::3 and
'ht 800: order 1' gives 800::1. Take a look at this block in cls_u32
if (err == 0) {
struct tc_u_knode __rcu **ins;
struct tc_u_knode *pins;
ins = &ht->ht[TC_U32_HASH(handle)];
for (pins = rtnl_dereference(*ins); pins;
ins = &pins->next, pins = rtnl_dereference(*ins))
if (TC_U32_NODE(handle) <
TC_U32_NODE(pins->handle))
break;
RCU_INIT_POINTER(n->next, pins);
rcu_assign_pointer(*ins, n);
u32_replace_hw_knode(tp, n);
*arg = (unsigned long)n;
return 0;
If you leave ht and order off the tc cli I believe 'tc' just
picks some semi-arbitrary ones for you. I've been in the habit
of always specifying them even for software filters.
The default table id is essentially 0x800. Default bucket is 0.
"order" essentially is the filter id. And given you can link tables
(Nice work John!); essentially the ht:bucket:nodeid is an "address" to
a specific filter on a specific table and when makes sense a specific
hash bucket. Some other way to look at it is as a way to construct
a mapping to a TCAM key.
Sure as long as your mapping works/looks like the software only case
it doesn't matter how you do the hardware mapping and my guess is this
is going to be highly device specific. Even with the handful of devices
I have it looks different depending on the device.
Note if you don't do the table links there really is no safe
way to get into the headers beyond the IP header because you need the
nexthdr stuff. Also just to stick this out there I have a patch to let
cls_u32 start processing at the ethernet header instead of the
transport header similar to how bpf works. This negative offset business
doesn't really work as best I can tell.
What John is doing is essentially taking the nodeid and trying to use
it as a priority. In otherwise the abstraction is reduced to a linked
list in which the ordering is how the list is traversed.
It may work in this case, but i am for being able to explicitly specify
priorities.
But that doesn't exist in software today. If users want explicit order
today they build the ht, nodeid out correctly just use that. If you
are working on a TCAM just use the nodeid that should be equivalent to
priority.
And although the ixgbe supports only a single table the mapping on more
complex devices will take it onto multiple tables if that optimizes
things.
Note I need to do a couple fixes on my existing code one to abort when
given a bad ifindex and two to hard abort when a hashtable is given
a higher order rule that I can't support. Both are fairly small tweaks
to the ixgbe code not to the infrastructure.
From: Amir Vadai" <hidden> Date: 2016-02-04 07:27:52
On Wed, Feb 03, 2016 at 01:29:59AM -0800, John Fastabend wrote:
quoted hunk
This adds initial support for offloading the u32 tc classifier. This
initial implementation only implements a few base matches and actions
to illustrate the use of the infrastructure patches.
However it is an interesting subset because it handles the u32 next
hdr logic to correctly map tcp packets from ip headers using the ihl
and protocol fields. After this is accepted we can extend the match
and action fields easily by updating the model header file.
Also only the drop action is supported initially.
Here is a short test script,
#tc qdisc add dev eth4 ingress
#tc filter add dev eth4 parent ffff: protocol ip \
u32 ht 800: order 1 \
match ip dst 15.0.0.1/32 match ip src 15.0.0.2/32 action drop
<-- hardware has dst/src ip match rule installed -->
#tc filter del dev eth4 parent ffff: prio 49152
#tc filter add dev eth4 parent ffff: protocol ip prio 99 \
handle 1: u32 divisor 1
#tc filter add dev eth4 protocol ip parent ffff: prio 99 \
u32 ht 800: order 1 link 1: \
offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6 ff
#tc filter add dev eth4 parent ffff: protocol ip \
u32 ht 1: order 3 match tcp src 23 ffff action drop
<-- hardware has tcp src port rule installed -->
#tc qdisc del dev eth4 parent ffff:
<-- hardware cleaned up -->
Signed-off-by: John Fastabend <redacted>
---
drivers/net/ethernet/intel/ixgbe/ixgbe.h | 3
drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 6 -
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 196 ++++++++++++++++++++++
3 files changed, 198 insertions(+), 7 deletions(-)
From: "Fastabend, John R" <john.fastabend@gmail.com> Date: 2016-02-04 08:23:09
On 2/3/2016 11:30 PM, Amir Vadai" wrote:
On Wed, Feb 03, 2016 at 01:29:59AM -0800, John Fastabend wrote:
quoted
This adds initial support for offloading the u32 tc classifier. This
initial implementation only implements a few base matches and actions
to illustrate the use of the infrastructure patches.
However it is an interesting subset because it handles the u32 next
hdr logic to correctly map tcp packets from ip headers using the ihl
and protocol fields. After this is accepted we can extend the match
and action fields easily by updating the model header file.
Also only the drop action is supported initially.
Here is a short test script,
#tc qdisc add dev eth4 ingress
#tc filter add dev eth4 parent ffff: protocol ip \
u32 ht 800: order 1 \
match ip dst 15.0.0.1/32 match ip src 15.0.0.2/32 action drop
<-- hardware has dst/src ip match rule installed -->
#tc filter del dev eth4 parent ffff: prio 49152
#tc filter add dev eth4 parent ffff: protocol ip prio 99 \
handle 1: u32 divisor 1
#tc filter add dev eth4 protocol ip parent ffff: prio 99 \
u32 ht 800: order 1 link 1: \
offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6 ff
#tc filter add dev eth4 parent ffff: protocol ip \
u32 ht 1: order 3 match tcp src 23 ffff action drop
<-- hardware has tcp src port rule installed -->
#tc qdisc del dev eth4 parent ffff:
<-- hardware cleaned up -->
Signed-off-by: John Fastabend <redacted>
---
drivers/net/ethernet/intel/ixgbe/ixgbe.h | 3
drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 6 -
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 196 ++++++++++++++++++++++
3 files changed, 198 insertions(+), 7 deletions(-)
@@ -8277,6 +8465,7 @@ static int ixgbe_set_features(struct net_device *netdev, */ switch (features & NETIF_F_NTUPLE) { case NETIF_F_NTUPLE:+ case NETIF_F_HW_TC: /* turn off ATR, enable perfect filters and reset */ if (!(adapter->flags & IXGBE_FLAG_FDIR_PERFECT_CAPABLE)) need_reset = true;
I think you have a bug here. I don't see how the NETIF_F_HW_TC case will
happen after masking 'features' out.
Ah I should have annotated this in the commit msg. I turn the feature
off by default to enable it the user needs to run
# ethtool -K ethx hw-tc-offload on
this is just a habit of mine to leave new features off by default for
a bit until I work out some of the kinks. For example I found a case
today where if you build loops into your u32 graph the hardware tables
can get out of sync with the software tables. This is sort of extreme
corner case not sure if anyone would really use u32 but it is valid
and the hardware should abort correctly.
Thanks,
John
Wed, Feb 03, 2016 at 10:27:32AM CET, john.fastabend@gmail.com wrote:
This extends the setup_tc framework so it can support more than just
the mqprio offload and push other classifiers and qdiscs into the
hardware. The series here targets the u32 classifier and ixgbe
driver. I worked out the u32 classifier because it is protocol
oblivious and aligns with multiple hardware devices I have access
to. I did an initial implementation on ixgbe because (a) I have one
in my box (b) its a stable driver and (c) it is relatively simple
compared to the other devices I have here but still has enough
flexibility to exercise the features of cls_u32.
I intentionally limited the scope of this series to the basic
feature set. Specifically this uses a 'big hammer' feature bit
to do the offload or not. If the bit is set you get offloaded rules
if it is not then rules will not be offloaded. If we can agree on
this patch series there are some more patches on my queue we can
talk about to make the offload decision per rule using flags similar
to how we do l2 mac updates. Additionally the error strategy can
be improved to be hard aborting, log and continue, etc. I think
these are nice to have improvements but shouldn't block this series.
Also by adding get_parse_graph and set_parse_graph attributes as
in my previous flow_api work we can build programmable devices
and programmatically learn when rules can or can not be loaded
into the hardware. Again future work.
Any comments/feedback appreciated.
I like this being thin and elegant solution. However, ~2 years ago when I
pushed openvswitch kernel datapath offload patchset, people were yelling
at me that it is not generic enough solution, that tc has to be able
to use the api (Jamal :)), nftables as well.
Now this patch is making offload strictly tc-based and nobody seems to
care :) I do. I think that we might try to find some generic middle layer.
Let's discuss this more in person next week.
Thanks!
From: Amir Vadai" <hidden> Date: 2016-02-04 12:09:10
On Thu, Feb 04, 2016 at 12:23:02AM -0800, Fastabend, John R wrote:
On 2/3/2016 11:30 PM, Amir Vadai" wrote:
quoted
On Wed, Feb 03, 2016 at 01:29:59AM -0800, John Fastabend wrote:
quoted
This adds initial support for offloading the u32 tc classifier. This
initial implementation only implements a few base matches and actions
to illustrate the use of the infrastructure patches.
However it is an interesting subset because it handles the u32 next
hdr logic to correctly map tcp packets from ip headers using the ihl
and protocol fields. After this is accepted we can extend the match
and action fields easily by updating the model header file.
Also only the drop action is supported initially.
Here is a short test script,
#tc qdisc add dev eth4 ingress
#tc filter add dev eth4 parent ffff: protocol ip \
u32 ht 800: order 1 \
match ip dst 15.0.0.1/32 match ip src 15.0.0.2/32 action drop
<-- hardware has dst/src ip match rule installed -->
#tc filter del dev eth4 parent ffff: prio 49152
#tc filter add dev eth4 parent ffff: protocol ip prio 99 \
handle 1: u32 divisor 1
#tc filter add dev eth4 protocol ip parent ffff: prio 99 \
u32 ht 800: order 1 link 1: \
offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6 ff
#tc filter add dev eth4 parent ffff: protocol ip \
u32 ht 1: order 3 match tcp src 23 ffff action drop
<-- hardware has tcp src port rule installed -->
#tc qdisc del dev eth4 parent ffff:
<-- hardware cleaned up -->
Signed-off-by: John Fastabend <redacted>
---
drivers/net/ethernet/intel/ixgbe/ixgbe.h | 3
drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 6 -
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 196 ++++++++++++++++++++++
3 files changed, 198 insertions(+), 7 deletions(-)
@@ -8277,6 +8465,7 @@ static int ixgbe_set_features(struct net_device *netdev, */ switch (features & NETIF_F_NTUPLE) { case NETIF_F_NTUPLE:+ case NETIF_F_HW_TC: /* turn off ATR, enable perfect filters and reset */ if (!(adapter->flags & IXGBE_FLAG_FDIR_PERFECT_CAPABLE)) need_reset = true;
I think you have a bug here. I don't see how the NETIF_F_HW_TC case will
happen after masking 'features' out.
Ah I should have annotated this in the commit msg. I turn the feature
off by default to enable it the user needs to run
# ethtool -K ethx hw-tc-offload on
this is just a habit of mine to leave new features off by default for
a bit until I work out some of the kinks. For example I found a case
today where if you build loops into your u32 graph the hardware tables
can get out of sync with the software tables. This is sort of extreme
corner case not sure if anyone would really use u32 but it is valid
and the hardware should abort correctly.
Yeh - that is nice :) But I was just pointing out on a small typo which I
think you have.
The new case will never happen. You compare: (features & NETIF_F_NTUPLE) == NETIF_F_HW_TC
Also the comment before the switch should be modified.
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2016-02-04 13:12:23
On 16-02-03 01:48 PM, Fastabend, John R wrote:
BTW: For the record John, I empathize with you that we need to
move. Please have patience - we are close; lets just get this resolved
in Seville. I like your patches a lot and would love to just have
your patches pushed in, but the challenges with community is being able
to reach some middle ground. We are not as bad as some of the standards
organizations. I am sure we'll get this resolved by end of next week
if not, I am %100 in agreement some form of your patches (And Amir's
need to go in and then we can refactor as needed)
quoted
1) "priorities" for filters and some form of "index" for actions is
is needed. I think index (which tends to be a 32 bit value is what
Amir's patches refered to as "cookie" - or at least some hardware
can be used to query the action with). Priorities maybe implicit in
the order in which they are added. And th idea of appending vs
exclusivity vs replace (which netlink already supports)
is important to worry about (TCAMS tend to assume an append mode
for example).
The code denotes add/del/replace already. I'm not sure why a TCAM
would assume an append mode but OK maybe that is some API you have
the APIs I use don't have these semantics.
Basically most hardware (or i should say driver implementations of
mostly TCAMS) allow you to add exactly the same filter as many times
as you want. They dont really look at what you want to filter on
and then scream "conflict". IOW, you (user) are responsible for
conflict resolution at the filter level. The driver sees this blob
and requests for some index/key from the hardware then just adds it.
You can then use this key/index to delete/replace etc.
This is what i meant by "append" mode.
However if a classifier implementation cares about filter ambiguity
resolution, then priorities are used. We need to worry about the
bigger picture.
For this series using cls_u32 the handle gives you everything you need
to put entries in the right table and row. Namely the ht # and order #
from 'tc'.
True - but with a caveat. There are only 2^12 max tables you can
have for example and up to 2^12 filters per bucket etc.
Take a look at u32_change and u32_classify its the handle
that places the filter into the list and the handle that is matched in
classify. We should place the filters in the hardware in the same order
that is used by u32_change.
I can see some parallels, but:
The nodeid in itself is insufficent for two reasons:
You cant have more than 2^12 filters per bucket;
and the nodeid then takes two meanings: a) it is an id
b) it specifies the order in which things are looked up.
I think you need to take the u32 address and map it to something in your
hardware. But at the same time it is important to have the abstraction
closely emulate your hardware.
Also ran a few tests and can't see how priority works in u32 maybe you
can shed some light but as best I can tell it doesn't have any effect
on rule execution.
True.
u32 doesnt care because it will give you a nodeid if you dont specify
one. i.e conflict resolution is mapped to you not specifying exactly
the same ht:bkt:nodeid more than once. And if you will let the
kernel do it for you (as i am assumming you are saying your hardware
will) then no need.
quoted
2) I like the u32 approach where it makes sense; but sometimes it
doesnt make sense from a usability pov. I work with some ASICs
that have 10 tuples that are fixed. Yes, a user can describe a policy
with u32 but flower would be more usable say with flower (both
programmatic and cli)
Sure so create a set of offload hooks for flower we don't need only
one hardware classifier any more than we would like a single software
classifiers.
Glad to hear that.
I was a little concerned that despite my love for u32 it was
going to be _the_ classifier. It doesnt fit for all offload cases
and sometimes it is because of human operators (the 10 tuple
hardware classifier i mentioned earlier).
BTW: Classifier in this case is very wide ranging (a regex hardware
offload for example qualifies).
Again I'm trying to faithfully implement what we have in software
and load that into the hardware. The handle today gives ingress/egres
hook. If you want an all ports hook we should add it to 'tc' software
first and then push that to the hardware not create magic hardware
bits. See I've drank the cool aid software first than hardware.
;-> No disagreement. It felt like a small sensible
change - thats why i suggested it.
quoted
4) Why are we forsaking switchdev John?
This is certainly re-usable beyond NICs and SRIOV.
Sure and switchdev can use it just like they use fdb_add and friends.
I just don't want to require switchdev infrastructure on things that
really are not switches. I think Amir indicated he would take a try
at the switchdev integration. If not I'm willing to do it but it
doesn't block this series in any way imo.
Ok. Makes sense.
quoted
5)What happened to being both able to hardware and/or software?
Follow up patch once we get the basic infrastructure in place with
the big feature flag bit. I have a patch I'm testing for this now
but again I want to move in logical and somewhat minimal sets.
Sounds sensible.
quoted
Anyways, I think Seville would be a blast! Come one, come all.
I'll be there but lets be sure to follow up with this online I
know folks are following this who wont be at Seville and I don't
see any reason to block these patches and stop the thread for a
week or more.
I really dont see much of a blocker.
cheers,
jamal
From: Amir Vadai" <hidden> Date: 2016-02-04 13:15:59
On Wed, Feb 03, 2016 at 01:28:37AM -0800, John Fastabend wrote:
quoted hunk
This patch allows netdev drivers to consume cls_u32 offloads via
the ndo_setup_tc ndo op.
This works aligns with how network drivers have been doing qdisc
offloads for mqprio.
Signed-off-by: John Fastabend <redacted>
---
include/linux/netdevice.h | 6 +++-
include/net/pkt_cls.h | 33 ++++++++++++++++++++
net/sched/cls_u32.c | 73 ++++++++++++++++++++++++++++++++++++++++++++-
3 files changed, 109 insertions(+), 3 deletions(-)
@@ -778,17 +778,21 @@ static inline bool netdev_phys_item_id_same(struct netdev_phys_item_id *a,typedefu16(*select_queue_fallback_t)(structnet_device*dev,structsk_buff*skb);-/* This structure holds attributes of qdisc and classifiers+/* These structures hold the attributes of qdisc and classifiers*thatarebeingpassedtothenetdevicethroughthesetup_tcop.*/enum{TC_SETUP_MQPRIO,+TC_SETUP_CLSU32,};+structtc_cls_u32_offload;+structtc_to_netdev{unsignedinttype;union{u8tc;+structtc_cls_u32_offload*cls_u32;};};
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2016-02-04 23:19:37
On Thu, Feb 04, 2016 at 10:16:56AM +0100, Jiri Pirko wrote:
Wed, Feb 03, 2016 at 10:27:32AM CET, john.fastabend@gmail.com wrote:
quoted
Also by adding get_parse_graph and set_parse_graph attributes as
in my previous flow_api work we can build programmable devices
and programmatically learn when rules can or can not be loaded
into the hardware. Again future work.
Any comments/feedback appreciated.
I like this being thin and elegant solution. However, ~2 years ago when I
pushed openvswitch kernel datapath offload patchset, people were yelling
at me that it is not generic enough solution, that tc has to be able
to use the api (Jamal :)), nftables as well.
I would be glad to join this debate during NetDev 1.1 too.
I think we should provide a solution that allows people uses both
tc and nftables, this would require a bit of generic infrastructure on
top of it so we don't restrict users to one single solution, in other
words, we allow the user to select its own poison.
Now this patch is making offload strictly tc-based and nobody seems to
care :) I do. I think that we might try to find some generic middle layer.
I agree and I'll be happy to help to push this ahead. Let's try to sit
and get together to resolve this.
See you soon.
From: "Fastabend, John R" <john.fastabend@gmail.com> Date: 2016-02-09 11:07:08
On 2/4/2016 3:19 PM, Pablo Neira Ayuso wrote:
On Thu, Feb 04, 2016 at 10:16:56AM +0100, Jiri Pirko wrote:
quoted
Wed, Feb 03, 2016 at 10:27:32AM CET, john.fastabend@gmail.com wrote:
quoted
Also by adding get_parse_graph and set_parse_graph attributes as
in my previous flow_api work we can build programmable devices
and programmatically learn when rules can or can not be loaded
into the hardware. Again future work.
Any comments/feedback appreciated.
Sorry if you get this twice it doesn't look like my original response
made it to netdev and the laptop I replied on charger blew up.
quoted
I like this being thin and elegant solution. However, ~2 years ago when I
pushed openvswitch kernel datapath offload patchset, people were yelling
at me that it is not generic enough solution, that tc has to be able
to use the api (Jamal :)), nftables as well.
The other problem with OVS is if you have the capabilities to do
wildcard lookups (e.g. TCAM/SRAM/etc) then offloading the exact
match table in OVS is really inefficient use of the resource. You
really want to load the megaflow table into hardware. I just don't
think its a good scheme for what you want.
I would be glad to join this debate during NetDev 1.1 too.
great.
I think we should provide a solution that allows people uses both
tc and nftables, this would require a bit of generic infrastructure on
top of it so we don't restrict users to one single solution, in other
words, we allow the user to select its own poison.
quoted
Now this patch is making offload strictly tc-based and nobody seems to
care :) I do. I think that we might try to find some generic middle
layer.
If we can build the universal model for 'tc' and 'nftable' we should
unify them higher in the stack? It doesn't make sense to me for the
driver folks to try and create the unified model for two subsystems
if we don't think its worthwhile in software as well.
I agree and I'll be happy to help to push this ahead. Let's try to sit
and get together to resolve this.
From: "Fastabend, John R" <john.fastabend@gmail.com> Date: 2016-02-09 11:09:56
On 2/4/2016 5:18 AM, Amir Vadai" wrote:
On Wed, Feb 03, 2016 at 01:28:37AM -0800, John Fastabend wrote:
quoted
This patch allows netdev drivers to consume cls_u32 offloads via
the ndo_setup_tc ndo op.
This works aligns with how network drivers have been doing qdisc
offloads for mqprio.
Signed-off-by: John Fastabend <redacted>
---
[...]
quoted
+enum {
+ TC_CLSU32_NEW_KNODE,
TC_CLSU32_NEW_KNODE is never used
aha yep that snuck in there. In a follow up patch for the fm10k devices
where we can support hash tables (e.g. divisor > 1) I use it. Although
on closer inspection I need to check that the divisor == 1 on ixgbe or
else abort because we can get out of sync if software expects hash
tables here.
Thanks, nice catch.
From: "Fastabend, John R" <john.fastabend@gmail.com> Date: 2016-02-09 11:24:41
On 2/4/2016 5:12 AM, Jamal Hadi Salim wrote:
On 16-02-03 01:48 PM, Fastabend, John R wrote:
BTW: For the record John, I empathize with you that we need to
move. Please have patience - we are close; lets just get this resolved
in Seville. I like your patches a lot and would love to just have
your patches pushed in, but the challenges with community is being able
to reach some middle ground. We are not as bad as some of the standards
organizations. I am sure we'll get this resolved by end of next week
if not, I am %100 in agreement some form of your patches (And Amir's
need to go in and then we can refactor as needed)
Agreed although I'm a bit worried we are starting to talk about a
single hardware IR. This discussion has always failed in my experience.
quoted
quoted
1) "priorities" for filters and some form of "index" for actions is
is needed. I think index (which tends to be a 32 bit value is what
Amir's patches refered to as "cookie" - or at least some hardware
can be used to query the action with). Priorities maybe implicit in
the order in which they are added. And th idea of appending vs
exclusivity vs replace (which netlink already supports)
is important to worry about (TCAMS tend to assume an append mode
for example).
The code denotes add/del/replace already. I'm not sure why a TCAM
would assume an append mode but OK maybe that is some API you have
the APIs I use don't have these semantics.
Basically most hardware (or i should say driver implementations of
mostly TCAMS) allow you to add exactly the same filter as many times
as you want. They dont really look at what you want to filter on
and then scream "conflict". IOW, you (user) are responsible for
conflict resolution at the filter level. The driver sees this blob
and requests for some index/key from the hardware then just adds it.
You can then use this key/index to delete/replace etc.
This is what i meant by "append" mode.
However if a classifier implementation cares about filter ambiguity
resolution, then priorities are used. We need to worry about the
bigger picture.
Sure in other classifiers its used but its not needed in the set I
planned to added it later.
quoted
For this series using cls_u32 the handle gives you everything you need
to put entries in the right table and row. Namely the ht # and order #
from 'tc'.
True - but with a caveat. There are only 2^12 max tables you can
have for example and up to 2^12 filters per bucket etc.
This is a software limitation as well right? If it hasn't showed up
as a limitation on the software side why would it be an issue here?
Do you have more than 2^12 tables on your devices? If so I guess we
can tack on another 32bits somewhere.
quoted
Take a look at u32_change and u32_classify its the handle
that places the filter into the list and the handle that is matched in
classify. We should place the filters in the hardware in the same order
that is used by u32_change.
I can see some parallels, but:
The nodeid in itself is insufficent for two reasons:
You cant have more than 2^12 filters per bucket;
and the nodeid then takes two meanings: a) it is an id
b) it specifies the order in which things are looked up.
I think you need to take the u32 address and map it to something in your
hardware. But at the same time it is important to have the abstraction
closely emulate your hardware.
IMO the hardware/interface must preserve the same ordering of
filters/hash_Tables/etc. How it does that mapping should be
a driver concern and it can always abort if it fails.
quoted
Also ran a few tests and can't see how priority works in u32 maybe you
can shed some light but as best I can tell it doesn't have any effect
on rule execution.
True.
u32 doesnt care because it will give you a nodeid if you dont specify
one. i.e conflict resolution is mapped to you not specifying exactly
the same ht:bkt:nodeid more than once. And if you will let the
kernel do it for you (as i am assumming you are saying your hardware
will) then no need.
Yep. Faithfully offloading u32 here not changing anything except
I do have to abort on some cases with the simpler devices. fm10k for
example can model hash nodes with divisors > 1.
quoted
quoted
2) I like the u32 approach where it makes sense; but sometimes it
doesnt make sense from a usability pov. I work with some ASICs
that have 10 tuples that are fixed. Yes, a user can describe a policy
with u32 but flower would be more usable say with flower (both
programmatic and cli)
Sure so create a set of offload hooks for flower we don't need only
one hardware classifier any more than we would like a single software
classifiers.
Glad to hear that.
I was a little concerned that despite my love for u32 it was
going to be _the_ classifier. It doesnt fit for all offload cases
and sometimes it is because of human operators (the 10 tuple
hardware classifier i mentioned earlier).
BTW: Classifier in this case is very wide ranging (a regex hardware
offload for example qualifies).
My issue is we can map flower onto u32 that is fine and u32 onto
bpf. But we lose a lot of the power of each classifier when we
do this. flower for example is nice because of its simplicity
presumably this translates into faster updates, u32 is great because
we get full parse graph support and hash tables, ebpf is the biggest
beast of all and lets us load arbitrary functions into the device.
All are nice in their own right.
quoted
Again I'm trying to faithfully implement what we have in software
and load that into the hardware. The handle today gives ingress/egres
hook. If you want an all ports hook we should add it to 'tc' software
first and then push that to the hardware not create magic hardware
bits. See I've drank the cool aid software first than hardware.
;-> No disagreement. It felt like a small sensible
change - thats why i suggested it.
Yep its in the git log if we can get past this initial series.
quoted
quoted
4) Why are we forsaking switchdev John?
This is certainly re-usable beyond NICs and SRIOV.
Sure and switchdev can use it just like they use fdb_add and friends.
I just don't want to require switchdev infrastructure on things that
really are not switches. I think Amir indicated he would take a try
at the switchdev integration. If not I'm willing to do it but it
doesn't block this series in any way imo.
Ok. Makes sense.
Great!
quoted
quoted
5)What happened to being both able to hardware and/or software?
Follow up patch once we get the basic infrastructure in place with
the big feature flag bit. I have a patch I'm testing for this now
but again I want to move in logical and somewhat minimal sets.
Sounds sensible.
quoted
quoted
Anyways, I think Seville would be a blast! Come one, come all.
I'll be there but lets be sure to follow up with this online I
know folks are following this who wont be at Seville and I don't
see any reason to block these patches and stop the thread for a
week or more.
I really dont see much of a blocker.
Perfect hopefully it didn't get thrashed on too much last couple
days. I'll be in Seville in a couple hours!
From: "Fastabend, John R" <john.fastabend@gmail.com> Date: 2016-02-09 11:27:42
[...]
quoted
Ah I should have annotated this in the commit msg. I turn the feature
off by default to enable it the user needs to run
# ethtool -K ethx hw-tc-offload on
this is just a habit of mine to leave new features off by default for
a bit until I work out some of the kinks. For example I found a case
today where if you build loops into your u32 graph the hardware tables
can get out of sync with the software tables. This is sort of extreme
corner case not sure if anyone would really use u32 but it is valid
and the hardware should abort correctly.
Yeh - that is nice :) But I was just pointing out on a small typo which I
think you have.
The new case will never happen. You compare: (features & NETIF_F_NTUPLE) == NETIF_F_HW_TC
Also the comment before the switch should be modified.
Aha nice catch my scripts were enabling both ntuple and hw-tc-offload
for testing compatibility issues. I wonder if there is a bug somewhere
else though that checks that code most likely because it was definately
getting offloaded.
Good catch again thanks.
From: "Fastabend, John R" <john.fastabend@gmail.com> Date: 2016-02-09 11:31:14
[...]
quoted
If you leave ht and order off the tc cli I believe 'tc' just
picks some semi-arbitrary ones for you. I've been in the habit
of always specifying them even for software filters.
The default table id is essentially 0x800. Default bucket is 0.
"order" essentially is the filter id. And given you can link tables
(Nice work John!); essentially the ht:bucket:nodeid is an "address" to
a specific filter on a specific table and when makes sense a specific
hash bucket. Some other way to look at it is as a way to construct
a mapping to a TCAM key.
What John is doing is essentially taking the nodeid and trying to use
it as a priority. In otherwise the abstraction is reduced to a linked
list in which the ordering is how the list is traversed.
It may work in this case, but i am for being able to explicitly specify
priorities.
Sorry bombing you with emails Jamal. Another thing to note is ixgbe
doesn't support hash tables explicitly but our other devices do. So
when a hash node is created we can map that onto a hardware block
and actually do the hash.
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2016-02-09 12:20:07
On 16-02-09 06:24 AM, Fastabend, John R wrote:
On 2/4/2016 5:12 AM, Jamal Hadi Salim wrote:
quoted
On 16-02-03 01:48 PM, Fastabend, John R wrote:
quoted
Basically most hardware (or i should say driver implementations of
mostly TCAMS) allow you to add exactly the same filter as many times
as you want. They dont really look at what you want to filter on
and then scream "conflict". IOW, you (user) are responsible for
conflict resolution at the filter level. The driver sees this blob
and requests for some index/key from the hardware then just adds it.
You can then use this key/index to delete/replace etc.
This is what i meant by "append" mode.
However if a classifier implementation cares about filter ambiguity
resolution, then priorities are used. We need to worry about the
bigger picture.
Sure in other classifiers its used but its not needed in the set I
planned to added it later.
If you leave it open for some other hardware to use we should be fine.
quoted
quoted
For this series using cls_u32 the handle gives you everything you need
to put entries in the right table and row. Namely the ht # and order #
from 'tc'.
True - but with a caveat. There are only 2^12 max tables you can
have for example and up to 2^12 filters per bucket etc.
This is a software limitation as well right? If it hasn't showed up
as a limitation on the software side why would it be an issue here?
Do you have more than 2^12 tables on your devices? If so I guess we
can tack on another 32bits somewhere.
That handle is used as an "Address" to the 32 bit filter.
Just beware of the semantics the handle has.
It hasnt shown up as a software limitation because the defaults
are good enough for most people. But if you ever want to install
a million rules that can be looked up at a reasonable pps rate
it will become very obvious quickly. I have a sample setup in the
talk tommorow which shows such an example.
quoted
I think you need to take the u32 address and map it to something in your
hardware. But at the same time it is important to have the abstraction
closely emulate your hardware.
IMO the hardware/interface must preserve the same ordering of
filters/hash_Tables/etc. How it does that mapping should be
a driver concern and it can always abort if it fails.
Sure.
quoted
quoted
Also ran a few tests and can't see how priority works in u32 maybe you
can shed some light but as best I can tell it doesn't have any effect
on rule execution.
True.
u32 doesnt care because it will give you a nodeid if you dont specify
one. i.e conflict resolution is mapped to you not specifying exactly
the same ht:bkt:nodeid more than once. And if you will let the
kernel do it for you (as i am assumming you are saying your hardware
will) then no need.
Yep. Faithfully offloading u32 here not changing anything except
I do have to abort on some cases with the simpler devices. fm10k for
example can model hash nodes with divisors > 1.
I wonder if when we get to capabilities we can do this...
quoted
My issue is we can map flower onto u32 that is fine and u32 onto
bpf. But we lose a lot of the power of each classifier when we
do this. flower for example is nice because of its simplicity
presumably this translates into faster updates, u32 is great because
we get full parse graph support and hash tables, ebpf is the biggest
beast of all and lets us load arbitrary functions into the device.
All are nice in their own right.