subject:"\[net\-next PATCH 7\/7\] net\: ixgbe\: add support for tc

Re: [net-next PATCH 7/7] net: ixgbe: add support for tc_u32 offload

2016-02-09 Thread Fastabend, John R

[...]

>> 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.

> 
>>
>> Thanks,
>> John
>>

Re: [net-next PATCH 7/7] net: ixgbe: add support for tc_u32 offload

2016-02-09 Thread Fastabend, John R

[...]

>>
>> 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.

> 
> cheers,
> jamal
>

Re: [net-next PATCH 7/7] net: ixgbe: add support for tc_u32 offload

2016-02-04 Thread Amir Vadai"

On Thu, Feb 04, 2016 at 12:23:02AM -0800, Fastabend, John R wrote:
> On 2/3/2016 11:30 PM, Amir Vadai" wrote:
> > 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 : 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 : prio 49152
> >>  #tc filter add dev eth4 parent : protocol ip prio 99 \
> >>handle 1: u32 divisor 1
> >>  #tc filter add dev eth4 protocol ip parent : 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 : protocol ip \
> >>u32 ht 1: order 3 match tcp src 23  action drop
> >>
> >> <-- hardware has tcp src port rule installed -->
> >>
> >>  #tc qdisc del dev eth4 parent :
> >>
> >> <-- hardware cleaned up -->
> >>
> >> Signed-off-by: John Fastabend 
> >> ---
> >>  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(-)
> >>
> >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h 
> >> b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> >> index 4b9156c..09c2d9b 100644
> >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> > 
> > [...]
> > 
> >> @@ -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.

> 
> Thanks,
> John
>

Re: [net-next PATCH 7/7] net: ixgbe: add support for tc_u32 offload

2016-02-04 Thread Fastabend, John R

On 2/3/2016 11:30 PM, Amir Vadai" wrote:
> 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 : 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 : prio 49152
>>  #tc filter add dev eth4 parent : protocol ip prio 99 \
>>  handle 1: u32 divisor 1
>>  #tc filter add dev eth4 protocol ip parent : 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 : protocol ip \
>>  u32 ht 1: order 3 match tcp src 23  action drop
>>
>> <-- hardware has tcp src port rule installed -->
>>
>>  #tc qdisc del dev eth4 parent :
>>
>> <-- hardware cleaned up -->
>>
>> Signed-off-by: John Fastabend 
>> ---
>>  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(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h 
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> index 4b9156c..09c2d9b 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> 
> [...]
> 
>> @@ -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

Re: [net-next PATCH 7/7] net: ixgbe: add support for tc_u32 offload

2016-02-03 Thread Amir Vadai"

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 : 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 : prio 49152
>  #tc filter add dev eth4 parent : protocol ip prio 99 \
>   handle 1: u32 divisor 1
>  #tc filter add dev eth4 protocol ip parent : 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 : protocol ip \
>   u32 ht 1: order 3 match tcp src 23  action drop
> 
> <-- hardware has tcp src port rule installed -->
> 
>  #tc qdisc del dev eth4 parent :
> 
> <-- hardware cleaned up -->
> 
> Signed-off-by: John Fastabend 
> ---
>  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?

[...]

>  
> -static int ixgbe_update_ethtool_fdir_entry(struct ixgbe_adapter *adapter,
> -struct ixgbe_fdir_filter *input,
> -u16 sw_idx)
> +int ixgbe_update_ethtool_fdir_entry(struct ixgbe_adapter *adapter,
> + struct ixgbe_fdir_filter *input,
> + u16 sw_idx)
>  {
>   struct ixgbe_hw *hw = >hw;
>   struct hlist_node *node2;
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c 
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 03e236c..a1a91bf 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -51,6 +51,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #ifdef CONFIG_OF
>  #include 
> @@ -8200,10 +8201,197 @@ int ixgbe_setup_tc(struct net_device *dev, u8 tc)
>   return 0;
>  }
>  
> +#include 
> +#include "ixgbe_model.h"
Did you leave those #include's in the middle of the file on purpose?

[...]

[net-next PATCH 7/7] net: ixgbe: add support for tc_u32 offload

2016-02-03 Thread John Fastabend

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 : 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 : prio 49152
 #tc filter add dev eth4 parent : protocol ip prio 99 \
handle 1: u32 divisor 1
 #tc filter add dev eth4 protocol ip parent : 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 : protocol ip \
u32 ht 1: order 3 match tcp src 23  action drop

<-- hardware has tcp src port rule installed -->

 #tc qdisc del dev eth4 parent :

<-- hardware cleaned up -->

Signed-off-by: John Fastabend 
---
 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(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h 
b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 4b9156c..09c2d9b 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -925,6 +925,9 @@ s32 ixgbe_fdir_erase_perfect_filter_82599(struct ixgbe_hw 
*hw,
  u16 soft_id);
 void ixgbe_atr_compute_perfect_hash_82599(union ixgbe_atr_input *input,
  union ixgbe_atr_input *mask);
+int ixgbe_update_ethtool_fdir_entry(struct ixgbe_adapter *adapter,
+   struct ixgbe_fdir_filter *input,
+   u16 sw_idx);
 void ixgbe_set_rx_mode(struct net_device *netdev);
 #ifdef CONFIG_IXGBE_DCB
 void ixgbe_set_rx_drop_en(struct ixgbe_adapter *adapter);
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c 
b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index bea96b3..726e0ee 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -2520,9 +2520,9 @@ static int ixgbe_get_rxnfc(struct net_device *dev, struct 
ethtool_rxnfc *cmd,
return ret;
 }
 
-static int ixgbe_update_ethtool_fdir_entry(struct ixgbe_adapter *adapter,
-  struct ixgbe_fdir_filter *input,
-  u16 sw_idx)
+int ixgbe_update_ethtool_fdir_entry(struct ixgbe_adapter *adapter,
+   struct ixgbe_fdir_filter *input,
+   u16 sw_idx)
 {
struct ixgbe_hw *hw = >hw;
struct hlist_node *node2;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c 
b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 03e236c..a1a91bf 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -51,6 +51,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #ifdef CONFIG_OF
 #include 
@@ -8200,10 +8201,197 @@ int ixgbe_setup_tc(struct net_device *dev, u8 tc)
return 0;
 }
 
+#include 
+#include "ixgbe_model.h"
+static int ixgbe_delete_clsu32(struct ixgbe_adapter *adapter,
+  struct tc_cls_u32_offload *cls)
+{
+   int err;
+
+   spin_lock(>fdir_perfect_lock);
+   err = ixgbe_update_ethtool_fdir_entry(adapter, NULL, cls->knode.handle);
+   spin_unlock(>fdir_perfect_lock);
+   return err;
+}
+
+#define IXGBE_MAX_LINK_HANDLE 10
+static struct ixgbe_mat_field *
+ixgbe_jump_tables[IXGBE_MAX_LINK_HANDLE] = {ixgbe_ipv4_fields,};
+
+static int ixgbe_configure_clsu32(struct ixgbe_adapter *adapter,
+ __be16 protocol,
+ struct tc_cls_u32_offload *cls)
+{
+   u32 loc = cls->knode.handle & 0xf;
+   struct ixgbe_hw *hw = >hw;
+   struct ixgbe_mat_field *field_ptr;
+   struct ixgbe_fdir_filter *input;
+   union ixgbe_atr_input mask;
+#ifdef CONFIG_NET_CLS_ACT
+   const struct tc_action *a;
+#endif
+   int i, err = 0;
+   u8 queue;
+   u32 handle;
+
+   memset(, 0, sizeof(union ixgbe_atr_input));
+   handle = cls->knode.handle;
+
+   /* At the moment cls_u32 jumps to transport layer and skips past
+* L2 headers. The canonical method to match L2 frames is to

Re: [net-next PATCH 7/7] net: ixgbe: add support for tc_u32 offload

2016-02-03 Thread John Fastabend

On 16-02-03 02:07 AM, Amir Vadai" wrote:
> 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 : 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 : prio 49152
>>  #tc filter add dev eth4 parent : protocol ip prio 99 \
>>  handle 1: u32 divisor 1
>>  #tc filter add dev eth4 protocol ip parent : 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 : protocol ip \
>>  u32 ht 1: order 3 match tcp src 23  action drop
>>
>> <-- hardware has tcp src port rule installed -->
>>
>>  #tc qdisc del dev eth4 parent :
>>
>> <-- hardware cleaned up -->
>>
>> Signed-off-by: John Fastabend 
>> ---
>>  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[TC_U32_HASH(handle)];
for (pins = rtnl_dereference(*ins); pins;
 ins = >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.

[...]

>>  
>> +#include 
>> +#include "ixgbe_model.h"
> Did you leave those #include's in the middle of the file on purpose?
> 
> [...]
> 

Probably bad form I'll move it to the top with the other header files.

Thanks!

Re: [net-next PATCH 7/7] net: ixgbe: add support for tc_u32 offload

2016-02-03 Thread Jamal Hadi Salim


On 16-02-03 05:26 AM, John Fastabend wrote:

On 16-02-03 02:07 AM, Amir Vadai" wrote:

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 : 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 : prio 49152
  #tc filter add dev eth4 parent : protocol ip prio 99 \
handle 1: u32 divisor 1
  #tc filter add dev eth4 protocol ip parent : 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 : protocol ip \
u32 ht 1: order 3 match tcp src 23  action drop

<-- hardware has tcp src port rule installed -->

  #tc qdisc del dev eth4 parent :

<-- hardware cleaned up -->

Signed-off-by: John Fastabend 
---
  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[TC_U32_HASH(handle)];
 for (pins = rtnl_dereference(*ins); pins;
  ins = >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

Re: [net-next PATCH 7/7] net: ixgbe: add support for tc_u32 offload

2016-02-03 Thread Amir Vadai"

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 : 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 : prio 49152
>  #tc filter add dev eth4 parent : protocol ip prio 99 \
>   handle 1: u32 divisor 1
>  #tc filter add dev eth4 protocol ip parent : 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 : protocol ip \
>   u32 ht 1: order 3 match tcp src 23  action drop
> 
> <-- hardware has tcp src port rule installed -->
> 
>  #tc qdisc del dev eth4 parent :
> 
> <-- hardware cleaned up -->
> 
> Signed-off-by: John Fastabend 
> ---
>  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(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h 
> b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> index 4b9156c..09c2d9b 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h

[...]

> @@ -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.

Re: [net-next PATCH 7/7] net: ixgbe: add support for tc_u32 offload

2016-02-03 Thread Fastabend, John R

On 2/3/2016 4:46 AM, Jamal Hadi Salim wrote:
[...]

>>>
>>> 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.

>>
>> 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[TC_U32_HASH(handle)];
>>  for (pins = rtnl_dereference(*ins); pins;
>>   ins = >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.

> 
> cheers,
> jamal
>

Re: [net-next PATCH 7/7] net: ixgbe: add support for tc_u32 offload

Re: [net-next PATCH 7/7] net: ixgbe: add support for tc_u32 offload

Re: [net-next PATCH 7/7] net: ixgbe: add support for tc_u32 offload

Re: [net-next PATCH 7/7] net: ixgbe: add support for tc_u32 offload

Re: [net-next PATCH 7/7] net: ixgbe: add support for tc_u32 offload

[net-next PATCH 7/7] net: ixgbe: add support for tc_u32 offload

Re: [net-next PATCH 7/7] net: ixgbe: add support for tc_u32 offload

Re: [net-next PATCH 7/7] net: ixgbe: add support for tc_u32 offload

Re: [net-next PATCH 7/7] net: ixgbe: add support for tc_u32 offload

Re: [net-next PATCH 7/7] net: ixgbe: add support for tc_u32 offload

10 matches

Site Navigation

Mail list logo

Footer information