[PATCH net-next] net: ixgbe: Fix cls_u32 offload support for ports and fields with masks.

[PATCH net-next] net: ixgbe: Fix cls_u32 offload support for ports and fields with masks.

[Sridhar Samudrala]

Fix support for 16 bit source/dest port matches in ixgbe model.
u32 uses a single 32-bit key value for both source and destination ports
starting at offset 0. So replace the 2 functions with a single function
that takes this key value/mask to program both source and dest ports.

Remove the incorrect check for mask in ixgbe_configure_clsu32()

Tested with the following filters:

 #tc qdisc add dev p4p1 ingress
 #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
	handle 800:0:1 u32 ht 800: \
	match ip dst 11.0.0.1/24 match ip src 11.0.0.2/24 action drop

 #tc filter del dev p4p1 parent ffff: protocol ip prio 99 \
	handle 800:0:1 u32
 #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
	handle 1: u32 divisor 1
 #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
	handle 800:0:10 u32 ht 800: link 1: \
	offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6 ff
 #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
	handle 1:0:10 u32 ht 1: \
	match tcp src 1024 ffff match tcp dst 80 ffff action drop

Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |  3 +--
 drivers/net/ethernet/intel/ixgbe/ixgbe_model.h | 17 ++++-------------
 2 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 10ccd96..c520f98 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -8348,8 +8348,7 @@ static int ixgbe_configure_clsu32(struct ixgbe_adapter *adapter,
 		int j;
 
 		for (j = 0; field_ptr[j].val; j++) {
-			if (field_ptr[j].off == off &&
-			    field_ptr[j].mask == m) {
+			if (field_ptr[j].off == off) {
 				field_ptr[j].val(input, &mask, val, m);
 				input->filter.formatted.flow_type |=
 					field_ptr[j].type;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_model.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_model.h
index ce48872..40d1730 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_model.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_model.h
@@ -65,28 +65,19 @@ static struct ixgbe_mat_field ixgbe_ipv4_fields[] = {
 	{ .val = NULL } /* terminal node */
 };
 
-static inline int ixgbe_mat_prgm_sport(struct ixgbe_fdir_filter *input,
+static inline int ixgbe_mat_prgm_ports(struct ixgbe_fdir_filter *input,
 				       union ixgbe_atr_input *mask,
 				       u32 val, u32 m)
 {
 	input->filter.formatted.src_port = val & 0xffff;
 	mask->formatted.src_port = m & 0xffff;
-	return 0;
-};
-
-static inline int ixgbe_mat_prgm_dport(struct ixgbe_fdir_filter *input,
-				       union ixgbe_atr_input *mask,
-				       u32 val, u32 m)
-{
-	input->filter.formatted.dst_port = val & 0xffff;
-	mask->formatted.dst_port = m & 0xffff;
+	input->filter.formatted.dst_port = val >> 16;
+	mask->formatted.dst_port = m  >> 16;
 	return 0;
 };
 
 static struct ixgbe_mat_field ixgbe_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,
+	{.off = 0, .mask = 0xffffffff, .val = ixgbe_mat_prgm_ports,
 	 .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
 	{ .val = NULL } /* terminal node */
 };
-- 
2.1.0

[Intel-wired-lan] [PATCH net-next] net: ixgbe: Fix cls_u32 offload support for ports and fields with masks.

[Sridhar Samudrala]

Fix support for 16 bit source/dest port matches in ixgbe model.
u32 uses a single 32-bit key value for both source and destination ports
starting at offset 0. So replace the 2 functions with a single function
that takes this key value/mask to program both source and dest ports.

Remove the incorrect check for mask in ixgbe_configure_clsu32()

Tested with the following filters:

 #tc qdisc add dev p4p1 ingress
 #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
	handle 800:0:1 u32 ht 800: \
	match ip dst 11.0.0.1/24 match ip src 11.0.0.2/24 action drop

 #tc filter del dev p4p1 parent ffff: protocol ip prio 99 \
	handle 800:0:1 u32
 #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
	handle 1: u32 divisor 1
 #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
	handle 800:0:10 u32 ht 800: link 1: \
	offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6 ff
 #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
	handle 1:0:10 u32 ht 1: \
	match tcp src 1024 ffff match tcp dst 80 ffff action drop

Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |  3 +--
 drivers/net/ethernet/intel/ixgbe/ixgbe_model.h | 17 ++++-------------
 2 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 10ccd96..c520f98 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -8348,8 +8348,7 @@ static int ixgbe_configure_clsu32(struct ixgbe_adapter *adapter,
 		int j;
 
 		for (j = 0; field_ptr[j].val; j++) {
-			if (field_ptr[j].off == off &&
-			    field_ptr[j].mask == m) {
+			if (field_ptr[j].off == off) {
 				field_ptr[j].val(input, &mask, val, m);
 				input->filter.formatted.flow_type |=
 					field_ptr[j].type;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_model.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_model.h
index ce48872..40d1730 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_model.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_model.h
@@ -65,28 +65,19 @@ static struct ixgbe_mat_field ixgbe_ipv4_fields[] = {
 	{ .val = NULL } /* terminal node */
 };
 
-static inline int ixgbe_mat_prgm_sport(struct ixgbe_fdir_filter *input,
+static inline int ixgbe_mat_prgm_ports(struct ixgbe_fdir_filter *input,
 				       union ixgbe_atr_input *mask,
 				       u32 val, u32 m)
 {
 	input->filter.formatted.src_port = val & 0xffff;
 	mask->formatted.src_port = m & 0xffff;
-	return 0;
-};
-
-static inline int ixgbe_mat_prgm_dport(struct ixgbe_fdir_filter *input,
-				       union ixgbe_atr_input *mask,
-				       u32 val, u32 m)
-{
-	input->filter.formatted.dst_port = val & 0xffff;
-	mask->formatted.dst_port = m & 0xffff;
+	input->filter.formatted.dst_port = val >> 16;
+	mask->formatted.dst_port = m  >> 16;
 	return 0;
 };
 
 static struct ixgbe_mat_field ixgbe_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,
+	{.off = 0, .mask = 0xffffffff, .val = ixgbe_mat_prgm_ports,
 	 .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
 	{ .val = NULL } /* terminal node */
 };
-- 
2.1.0

Re: [Intel-wired-lan] [PATCH net-next] net: ixgbe: Fix cls_u32 offload support for ports and fields with masks.

[John Fastabend]

On 16-03-04 11:47 AM, Sridhar Samudrala wrote:
> Fix support for 16 bit source/dest port matches in ixgbe model.
> u32 uses a single 32-bit key value for both source and destination ports
> starting at offset 0. So replace the 2 functions with a single function
> that takes this key value/mask to program both source and dest ports.
> 
> Remove the incorrect check for mask in ixgbe_configure_clsu32()
> 
> Tested with the following filters:
> 
>  #tc qdisc add dev p4p1 ingress
>  #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
> 	handle 800:0:1 u32 ht 800: \
> 	match ip dst 11.0.0.1/24 match ip src 11.0.0.2/24 action drop
> 
>  #tc filter del dev p4p1 parent ffff: protocol ip prio 99 \
> 	handle 800:0:1 u32
>  #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
> 	handle 1: u32 divisor 1
>  #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
> 	handle 800:0:10 u32 ht 800: link 1: \
> 	offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6 ff
>  #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
> 	handle 1:0:10 u32 ht 1: \
> 	match tcp src 1024 ffff match tcp dst 80 ffff action drop
> 
> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> ---

But this will break setting only dst port or only src port. Do we
actually need three signatures to match? Something like,

  static struct ixgbe_mat_field ixgbe_tcp_fields[] = {
	{.off = 0, .mask = 0xffffffff, .val = ixgbe_mat_prgm_ports,
  	 .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
	{.off = 0, .mask = 0xffff0000, .val = ixgbe_mat_prgm_dport,
  	 .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
	{.off = 0, .mask = 0x0000ffff, .val = ixgbe_mat_prgm_sport,
  	 .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
  	{ .val = NULL } /* terminal node */
  };

Also just a reminder if we get multiple fields in a ixgbe_mat_field
struct we need to abort out of the for loop in the cls_u32 configure
function. Actually we can probably just push that as its own patch
to make the core function more versatile/usable.

Thanks,
John

[Intel-wired-lan] [PATCH net-next] net: ixgbe: Fix cls_u32 offload support for ports and fields with masks.

[John Fastabend]

On 16-03-04 11:47 AM, Sridhar Samudrala wrote:
> Fix support for 16 bit source/dest port matches in ixgbe model.
> u32 uses a single 32-bit key value for both source and destination ports
> starting at offset 0. So replace the 2 functions with a single function
> that takes this key value/mask to program both source and dest ports.
> 
> Remove the incorrect check for mask in ixgbe_configure_clsu32()
> 
> Tested with the following filters:
> 
>  #tc qdisc add dev p4p1 ingress
>  #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
> 	handle 800:0:1 u32 ht 800: \
> 	match ip dst 11.0.0.1/24 match ip src 11.0.0.2/24 action drop
> 
>  #tc filter del dev p4p1 parent ffff: protocol ip prio 99 \
> 	handle 800:0:1 u32
>  #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
> 	handle 1: u32 divisor 1
>  #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
> 	handle 800:0:10 u32 ht 800: link 1: \
> 	offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6 ff
>  #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
> 	handle 1:0:10 u32 ht 1: \
> 	match tcp src 1024 ffff match tcp dst 80 ffff action drop
> 
> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> ---

But this will break setting only dst port or only src port. Do we
actually need three signatures to match? Something like,

  static struct ixgbe_mat_field ixgbe_tcp_fields[] = {
	{.off = 0, .mask = 0xffffffff, .val = ixgbe_mat_prgm_ports,
  	 .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
	{.off = 0, .mask = 0xffff0000, .val = ixgbe_mat_prgm_dport,
  	 .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
	{.off = 0, .mask = 0x0000ffff, .val = ixgbe_mat_prgm_sport,
  	 .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
  	{ .val = NULL } /* terminal node */
  };

Also just a reminder if we get multiple fields in a ixgbe_mat_field
struct we need to abort out of the for loop in the cls_u32 configure
function. Actually we can probably just push that as its own patch
to make the core function more versatile/usable.

Thanks,
John

Re: [Intel-wired-lan] [PATCH net-next] net: ixgbe: Fix cls_u32 offload support for ports and fields with masks.

[Samudrala, Sridhar]

On 3/4/2016 12:41 PM, John Fastabend wrote:
> On 16-03-04 11:47 AM, Sridhar Samudrala wrote:
>> Fix support for 16 bit source/dest port matches in ixgbe model.
>> u32 uses a single 32-bit key value for both source and destination ports
>> starting at offset 0. So replace the 2 functions with a single function
>> that takes this key value/mask to program both source and dest ports.
>>
>> Remove the incorrect check for mask in ixgbe_configure_clsu32()
>>
>> Tested with the following filters:
>>
>>   #tc qdisc add dev p4p1 ingress
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>> 	handle 800:0:1 u32 ht 800: \
>> 	match ip dst 11.0.0.1/24 match ip src 11.0.0.2/24 action drop
>>
>>   #tc filter del dev p4p1 parent ffff: protocol ip prio 99 \
>> 	handle 800:0:1 u32
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>> 	handle 1: u32 divisor 1
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>> 	handle 800:0:10 u32 ht 800: link 1: \
>> 	offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6 ff
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>> 	handle 1:0:10 u32 ht 1: \
>> 	match tcp src 1024 ffff match tcp dst 80 ffff action drop
>>
>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>> ---
> But this will break setting only dst port or only src port.

No. This will not break specifying only src or dst port. The value/mask for the
unspecified port will be set to zero. So it should be fine.

For ex:
	match tcp src 1024 ffff match tcp dst 80 ffff
		=> match 04000050/ffffffff at nexthdr+0
	match tcp src 1024 ffff
		=> match 04000000/ffff0000 at nexthdr+0
	match tcp dst 80 ffff
		=> match 00000050/0000ffff at nexthdr+0


>   Do we
> actually need three signatures to match? Something like,
>
>    static struct ixgbe_mat_field ixgbe_tcp_fields[] = {
> 	{.off = 0, .mask = 0xffffffff, .val = ixgbe_mat_prgm_ports,
>    	 .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
> 	{.off = 0, .mask = 0xffff0000, .val = ixgbe_mat_prgm_dport,
>    	 .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
> 	{.off = 0, .mask = 0x0000ffff, .val = ixgbe_mat_prgm_sport,
>    	 .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
>    	{ .val = NULL } /* terminal node */
>    };
>
> Also just a reminder if we get multiple fields in a ixgbe_mat_field
> struct we need to abort out of the for loop in the cls_u32 configure
> function. Actually we can probably just push that as its own patch
> to make the core function more versatile/usable.
>
> Thanks,
> John

[Intel-wired-lan] [PATCH net-next] net: ixgbe: Fix cls_u32 offload support for ports and fields with masks.

[Samudrala, Sridhar]

On 3/4/2016 12:41 PM, John Fastabend wrote:
> On 16-03-04 11:47 AM, Sridhar Samudrala wrote:
>> Fix support for 16 bit source/dest port matches in ixgbe model.
>> u32 uses a single 32-bit key value for both source and destination ports
>> starting at offset 0. So replace the 2 functions with a single function
>> that takes this key value/mask to program both source and dest ports.
>>
>> Remove the incorrect check for mask in ixgbe_configure_clsu32()
>>
>> Tested with the following filters:
>>
>>   #tc qdisc add dev p4p1 ingress
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>> 	handle 800:0:1 u32 ht 800: \
>> 	match ip dst 11.0.0.1/24 match ip src 11.0.0.2/24 action drop
>>
>>   #tc filter del dev p4p1 parent ffff: protocol ip prio 99 \
>> 	handle 800:0:1 u32
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>> 	handle 1: u32 divisor 1
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>> 	handle 800:0:10 u32 ht 800: link 1: \
>> 	offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6 ff
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>> 	handle 1:0:10 u32 ht 1: \
>> 	match tcp src 1024 ffff match tcp dst 80 ffff action drop
>>
>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>> ---
> But this will break setting only dst port or only src port.

No. This will not break specifying only src or dst port. The value/mask for the
unspecified port will be set to zero. So it should be fine.

For ex:
	match tcp src 1024 ffff match tcp dst 80 ffff
		=> match 04000050/ffffffff at nexthdr+0
	match tcp src 1024 ffff
		=> match 04000000/ffff0000 at nexthdr+0
	match tcp dst 80 ffff
		=> match 00000050/0000ffff at nexthdr+0


>   Do we
> actually need three signatures to match? Something like,
>
>    static struct ixgbe_mat_field ixgbe_tcp_fields[] = {
> 	{.off = 0, .mask = 0xffffffff, .val = ixgbe_mat_prgm_ports,
>    	 .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
> 	{.off = 0, .mask = 0xffff0000, .val = ixgbe_mat_prgm_dport,
>    	 .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
> 	{.off = 0, .mask = 0x0000ffff, .val = ixgbe_mat_prgm_sport,
>    	 .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
>    	{ .val = NULL } /* terminal node */
>    };
>
> Also just a reminder if we get multiple fields in a ixgbe_mat_field
> struct we need to abort out of the for loop in the cls_u32 configure
> function. Actually we can probably just push that as its own patch
> to make the core function more versatile/usable.
>
> Thanks,
> John

Re: [Intel-wired-lan] [PATCH net-next] net: ixgbe: Fix cls_u32 offload support for ports and fields with masks.

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1758 bytes --]

On Fri, 2016-03-04 at 11:47 -0800, Sridhar Samudrala wrote:
> Fix support for 16 bit source/dest port matches in ixgbe model.
> u32 uses a single 32-bit key value for both source and destination
> ports
> starting at offset 0. So replace the 2 functions with a single
> function
> that takes this key value/mask to program both source and dest ports.
> 
> Remove the incorrect check for mask in ixgbe_configure_clsu32()
> 
> Tested with the following filters:
> 
>  #tc qdisc add dev p4p1 ingress
>  #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>         handle 800:0:1 u32 ht 800: \
>         match ip dst 11.0.0.1/24 match ip src 11.0.0.2/24 action drop
> 
>  #tc filter del dev p4p1 parent ffff: protocol ip prio 99 \
>         handle 800:0:1 u32
>  #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>         handle 1: u32 divisor 1
>  #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>         handle 800:0:10 u32 ht 800: link 1: \
>         offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6
> ff
>  #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>         handle 1:0:10 u32 ht 1: \
>         match tcp src 1024 ffff match tcp dst 80 ffff action drop
> 
> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |  3 +--
>  drivers/net/ethernet/intel/ixgbe/ixgbe_model.h | 17 ++++----------
> ---
>  2 files changed, 5 insertions(+), 15 deletions(-)

Is this v2?  If not, why are you re-sending a patch that is already in
my queue?  If so, where is the changelog so we know what changed in
this updated v2 of the patch?

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[Intel-wired-lan] [PATCH net-next] net: ixgbe: Fix cls_u32 offload support for ports and fields with masks.

[Jeff Kirsher]

On Fri, 2016-03-04 at 11:47 -0800, Sridhar Samudrala wrote:
> Fix support for 16 bit source/dest port matches in ixgbe model.
> u32 uses a single 32-bit key value for both source and destination
> ports
> starting at offset 0. So replace the 2 functions with a single
> function
> that takes this key value/mask to program both source and dest ports.
> 
> Remove the incorrect check for mask in ixgbe_configure_clsu32()
> 
> Tested with the following filters:
> 
> ?#tc qdisc add dev p4p1 ingress
> ?#tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
> ????????handle 800:0:1 u32 ht 800: \
> ????????match ip dst 11.0.0.1/24 match ip src 11.0.0.2/24 action drop
> 
> ?#tc filter del dev p4p1 parent ffff: protocol ip prio 99 \
> ????????handle 800:0:1 u32
> ?#tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
> ????????handle 1: u32 divisor 1
> ?#tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
> ????????handle 800:0:10 u32 ht 800: link 1: \
> ????????offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6
> ff
> ?#tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
> ????????handle 1:0:10 u32 ht 1: \
> ????????match tcp src 1024 ffff match tcp dst 80 ffff action drop
> 
> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> ---
> ?drivers/net/ethernet/intel/ixgbe/ixgbe_main.c? |? 3 +--
> ?drivers/net/ethernet/intel/ixgbe/ixgbe_model.h | 17 ++++----------
> ---
> ?2 files changed, 5 insertions(+), 15 deletions(-)

Is this v2? ?If not, why are you re-sending a patch that is already in
my queue? ?If so, where is the changelog so we know what changed in
this updated v2 of the patch?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20160304/ec96557f/attachment.asc>

Re: [Intel-wired-lan] [PATCH net-next] net: ixgbe: Fix cls_u32 offload support for ports and fields with masks.

[Samudrala, Sridhar]

On 3/4/2016 2:10 PM, Jeff Kirsher wrote:
> On Fri, 2016-03-04 at 11:47 -0800, Sridhar Samudrala wrote:
>> Fix support for 16 bit source/dest port matches in ixgbe model.
>> u32 uses a single 32-bit key value for both source and destination
>> ports
>> starting at offset 0. So replace the 2 functions with a single
>> function
>> that takes this key value/mask to program both source and dest ports.
>>
>> Remove the incorrect check for mask in ixgbe_configure_clsu32()
>>
>> Tested with the following filters:
>>
>>   #tc qdisc add dev p4p1 ingress
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>          handle 800:0:1 u32 ht 800: \
>>          match ip dst 11.0.0.1/24 match ip src 11.0.0.2/24 action drop
>>
>>   #tc filter del dev p4p1 parent ffff: protocol ip prio 99 \
>>          handle 800:0:1 u32
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>          handle 1: u32 divisor 1
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>          handle 800:0:10 u32 ht 800: link 1: \
>>          offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6
>> ff
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>          handle 1:0:10 u32 ht 1: \
>>          match tcp src 1024 ffff match tcp dst 80 ffff action drop
>>
>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>> ---
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |  3 +--
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_model.h | 17 ++++----------
>> ---
>>   2 files changed, 5 insertions(+), 15 deletions(-)
> Is this v2?  If not, why are you re-sending a patch that is already in
> my queue?  If so, where is the changelog so we know what changed in
> this updated v2 of the patch?
No. This is another patch that fixes other issues on top of the previous 
one.

Thanks
Sridhar

[Intel-wired-lan] [PATCH net-next] net: ixgbe: Fix cls_u32 offload support for ports and fields with masks.

[Samudrala, Sridhar]

On 3/4/2016 2:10 PM, Jeff Kirsher wrote:
> On Fri, 2016-03-04 at 11:47 -0800, Sridhar Samudrala wrote:
>> Fix support for 16 bit source/dest port matches in ixgbe model.
>> u32 uses a single 32-bit key value for both source and destination
>> ports
>> starting at offset 0. So replace the 2 functions with a single
>> function
>> that takes this key value/mask to program both source and dest ports.
>>
>> Remove the incorrect check for mask in ixgbe_configure_clsu32()
>>
>> Tested with the following filters:
>>
>>   #tc qdisc add dev p4p1 ingress
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>          handle 800:0:1 u32 ht 800: \
>>          match ip dst 11.0.0.1/24 match ip src 11.0.0.2/24 action drop
>>
>>   #tc filter del dev p4p1 parent ffff: protocol ip prio 99 \
>>          handle 800:0:1 u32
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>          handle 1: u32 divisor 1
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>          handle 800:0:10 u32 ht 800: link 1: \
>>          offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6
>> ff
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>          handle 1:0:10 u32 ht 1: \
>>          match tcp src 1024 ffff match tcp dst 80 ffff action drop
>>
>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>> ---
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |  3 +--
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_model.h | 17 ++++----------
>> ---
>>   2 files changed, 5 insertions(+), 15 deletions(-)
> Is this v2?  If not, why are you re-sending a patch that is already in
> my queue?  If so, where is the changelog so we know what changed in
> this updated v2 of the patch?
No. This is another patch that fixes other issues on top of the previous 
one.

Thanks
Sridhar

[Intel-wired-lan] [PATCH net-next] net: ixgbe: Fix cls_u32 offload support for ports and fields with masks.

[John Fastabend]

On 16-03-04 01:27 PM, Samudrala, Sridhar wrote:
> On 3/4/2016 12:41 PM, John Fastabend wrote:
>> On 16-03-04 11:47 AM, Sridhar Samudrala wrote:
>>> Fix support for 16 bit source/dest port matches in ixgbe model.
>>> u32 uses a single 32-bit key value for both source and destination ports
>>> starting at offset 0. So replace the 2 functions with a single function
>>> that takes this key value/mask to program both source and dest ports.
>>>
>>> Remove the incorrect check for mask in ixgbe_configure_clsu32()
>>>
>>> Tested with the following filters:
>>>
>>>   #tc qdisc add dev p4p1 ingress
>>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>>     handle 800:0:1 u32 ht 800: \
>>>     match ip dst 11.0.0.1/24 match ip src 11.0.0.2/24 action drop
>>>
>>>   #tc filter del dev p4p1 parent ffff: protocol ip prio 99 \
>>>     handle 800:0:1 u32
>>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>>     handle 1: u32 divisor 1
>>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>>     handle 800:0:10 u32 ht 800: link 1: \
>>>     offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6 ff
>>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>>     handle 1:0:10 u32 ht 1: \
>>>     match tcp src 1024 ffff match tcp dst 80 ffff action drop
>>>
>>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>>> ---
>> But this will break setting only dst port or only src port.
> 
> No. This will not break specifying only src or dst port. The value/mask
> for the
> unspecified port will be set to zero. So it should be fine.
> 
> For ex:
>     match tcp src 1024 ffff match tcp dst 80 ffff
>         => match 04000050/ffffffff at nexthdr+0
>     match tcp src 1024 ffff
>         => match 04000000/ffff0000 at nexthdr+0
>     match tcp dst 80 ffff
>         => match 00000050/0000ffff at nexthdr+0
> 
> 
>>   Do we
>> actually need three signatures to match? Something like,
>>
>>    static struct ixgbe_mat_field ixgbe_tcp_fields[] = {
>>     {.off = 0, .mask = 0xffffffff, .val = ixgbe_mat_prgm_ports,
>>         .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
>>     {.off = 0, .mask = 0xffff0000, .val = ixgbe_mat_prgm_dport,
>>         .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
>>     {.off = 0, .mask = 0x0000ffff, .val = ixgbe_mat_prgm_sport,
>>         .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
>>        { .val = NULL } /* terminal node */
>>    };
>>
>> Also just a reminder if we get multiple fields in a ixgbe_mat_field
>> struct we need to abort out of the for loop in the cls_u32 configure
>> function. Actually we can probably just push that as its own patch
>> to make the core function more versatile/usable.
>>
>> Thanks,
>> John
> 

Dropped netdev no reason to push mail to netdev that has to do with our
driver internals.

OK I see so you dropped the .mask check in fact you actually put it
there in the commit message I just missed the detail/implication.

I think this is fine and it allows supporting mask entries now which
I blocked in the initial submission. But with this is there any reason
to have a mask field in ixgbe_mat_field? We could probably do this with
two patches, one to drop the 'mask' field and check and another to fix
the ixgbe_tcp_fields patch?

Thanks,
John

[Intel-wired-lan] [PATCH net-next] net: ixgbe: Fix cls_u32 offload support for ports and fields with masks.

[Samudrala, Sridhar]

On 3/4/2016 2:22 PM, John Fastabend wrote:
> On 16-03-04 01:27 PM, Samudrala, Sridhar wrote:
>> On 3/4/2016 12:41 PM, John Fastabend wrote:
>>> On 16-03-04 11:47 AM, Sridhar Samudrala wrote:
>>>> Fix support for 16 bit source/dest port matches in ixgbe model.
>>>> u32 uses a single 32-bit key value for both source and destination ports
>>>> starting at offset 0. So replace the 2 functions with a single function
>>>> that takes this key value/mask to program both source and dest ports.
>>>>
>>>> Remove the incorrect check for mask in ixgbe_configure_clsu32()
>>>>
>>>> Tested with the following filters:
>>>>
>>>>    #tc qdisc add dev p4p1 ingress
>>>>    #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>>>      handle 800:0:1 u32 ht 800: \
>>>>      match ip dst 11.0.0.1/24 match ip src 11.0.0.2/24 action drop
>>>>
>>>>    #tc filter del dev p4p1 parent ffff: protocol ip prio 99 \
>>>>      handle 800:0:1 u32
>>>>    #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>>>      handle 1: u32 divisor 1
>>>>    #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>>>      handle 800:0:10 u32 ht 800: link 1: \
>>>>      offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6 ff
>>>>    #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>>>      handle 1:0:10 u32 ht 1: \
>>>>      match tcp src 1024 ffff match tcp dst 80 ffff action drop
>>>>
>>>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>>>> ---
>>> But this will break setting only dst port or only src port.
>> No. This will not break specifying only src or dst port. The value/mask
>> for the
>> unspecified port will be set to zero. So it should be fine.
>>
>> For ex:
>>      match tcp src 1024 ffff match tcp dst 80 ffff
>>          => match 04000050/ffffffff at nexthdr+0
>>      match tcp src 1024 ffff
>>          => match 04000000/ffff0000 at nexthdr+0
>>      match tcp dst 80 ffff
>>          => match 00000050/0000ffff at nexthdr+0
>>
>>
>>>    Do we
>>> actually need three signatures to match? Something like,
>>>
>>>     static struct ixgbe_mat_field ixgbe_tcp_fields[] = {
>>>      {.off = 0, .mask = 0xffffffff, .val = ixgbe_mat_prgm_ports,
>>>          .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
>>>      {.off = 0, .mask = 0xffff0000, .val = ixgbe_mat_prgm_dport,
>>>          .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
>>>      {.off = 0, .mask = 0x0000ffff, .val = ixgbe_mat_prgm_sport,
>>>          .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
>>>         { .val = NULL } /* terminal node */
>>>     };
>>>
>>> Also just a reminder if we get multiple fields in a ixgbe_mat_field
>>> struct we need to abort out of the for loop in the cls_u32 configure
>>> function. Actually we can probably just push that as its own patch
>>> to make the core function more versatile/usable.
>>>
>>> Thanks,
>>> John
> Dropped netdev no reason to push mail to netdev that has to do with our
> driver internals.
>
> OK I see so you dropped the .mask check in fact you actually put it
> there in the commit message I just missed the detail/implication.
>
> I think this is fine and it allows supporting mask entries now which
> I blocked in the initial submission. But with this is there any reason
> to have a mask field in ixgbe_mat_field? We could probably do this with
> two patches, one to drop the 'mask' field and check and another to fix
> the ixgbe_tcp_fields patch?

OK. will remove the 'mask' field and resubmit as 2 patches.

Thanks
Sridhar

[Intel-wired-lan] [PATCH net-next] net: ixgbe: Fix cls_u32 offload support for ports and fields with masks.

[John Fastabend]

On 16-03-04 03:07 PM, Samudrala, Sridhar wrote:
> 
> 
> On 3/4/2016 2:22 PM, John Fastabend wrote:
>> On 16-03-04 01:27 PM, Samudrala, Sridhar wrote:
>>> On 3/4/2016 12:41 PM, John Fastabend wrote:
>>>> On 16-03-04 11:47 AM, Sridhar Samudrala wrote:
>>>>> Fix support for 16 bit source/dest port matches in ixgbe model.
>>>>> u32 uses a single 32-bit key value for both source and destination
>>>>> ports
>>>>> starting at offset 0. So replace the 2 functions with a single
>>>>> function
>>>>> that takes this key value/mask to program both source and dest ports.
>>>>>
>>>>> Remove the incorrect check for mask in ixgbe_configure_clsu32()
>>>>>
>>>>> Tested with the following filters:
>>>>>
>>>>>    #tc qdisc add dev p4p1 ingress
>>>>>    #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>>>>      handle 800:0:1 u32 ht 800: \
>>>>>      match ip dst 11.0.0.1/24 match ip src 11.0.0.2/24 action drop
>>>>>
>>>>>    #tc filter del dev p4p1 parent ffff: protocol ip prio 99 \
>>>>>      handle 800:0:1 u32
>>>>>    #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>>>>      handle 1: u32 divisor 1
>>>>>    #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>>>>      handle 800:0:10 u32 ht 800: link 1: \
>>>>>      offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6 ff
>>>>>    #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>>>>      handle 1:0:10 u32 ht 1: \
>>>>>      match tcp src 1024 ffff match tcp dst 80 ffff action drop
>>>>>
>>>>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>>>>> ---
>>>> But this will break setting only dst port or only src port.
>>> No. This will not break specifying only src or dst port. The value/mask
>>> for the
>>> unspecified port will be set to zero. So it should be fine.
>>>
>>> For ex:
>>>      match tcp src 1024 ffff match tcp dst 80 ffff
>>>          => match 04000050/ffffffff at nexthdr+0
>>>      match tcp src 1024 ffff
>>>          => match 04000000/ffff0000 at nexthdr+0
>>>      match tcp dst 80 ffff
>>>          => match 00000050/0000ffff at nexthdr+0
>>>
>>>
>>>>    Do we
>>>> actually need three signatures to match? Something like,
>>>>
>>>>     static struct ixgbe_mat_field ixgbe_tcp_fields[] = {
>>>>      {.off = 0, .mask = 0xffffffff, .val = ixgbe_mat_prgm_ports,
>>>>          .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
>>>>      {.off = 0, .mask = 0xffff0000, .val = ixgbe_mat_prgm_dport,
>>>>          .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
>>>>      {.off = 0, .mask = 0x0000ffff, .val = ixgbe_mat_prgm_sport,
>>>>          .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
>>>>         { .val = NULL } /* terminal node */
>>>>     };
>>>>
>>>> Also just a reminder if we get multiple fields in a ixgbe_mat_field
>>>> struct we need to abort out of the for loop in the cls_u32 configure
>>>> function. Actually we can probably just push that as its own patch
>>>> to make the core function more versatile/usable.
>>>>
>>>> Thanks,
>>>> John
>> Dropped netdev no reason to push mail to netdev that has to do with our
>> driver internals.
>>
>> OK I see so you dropped the .mask check in fact you actually put it
>> there in the commit message I just missed the detail/implication.
>>
>> I think this is fine and it allows supporting mask entries now which
>> I blocked in the initial submission. But with this is there any reason
>> to have a mask field in ixgbe_mat_field? We could probably do this with
>> two patches, one to drop the 'mask' field and check and another to fix
>> the ixgbe_tcp_fields patch?
> 
> OK. will remove the 'mask' field and resubmit as 2 patches.
> 
> Thanks
> Sridhar

Great thanks nice catch / improvements.

[Intel-wired-lan] [PATCH net-next] net: ixgbe: Fix cls_u32 offload support for ports and fields with masks.

[John Fastabend]

On 16-03-04 03:07 PM, Samudrala, Sridhar wrote:
> 
> 
> On 3/4/2016 2:22 PM, John Fastabend wrote:
>> On 16-03-04 01:27 PM, Samudrala, Sridhar wrote:
>>> On 3/4/2016 12:41 PM, John Fastabend wrote:
>>>> On 16-03-04 11:47 AM, Sridhar Samudrala wrote:
>>>>> Fix support for 16 bit source/dest port matches in ixgbe model.
>>>>> u32 uses a single 32-bit key value for both source and destination
>>>>> ports
>>>>> starting at offset 0. So replace the 2 functions with a single
>>>>> function
>>>>> that takes this key value/mask to program both source and dest ports.
>>>>>
>>>>> Remove the incorrect check for mask in ixgbe_configure_clsu32()
>>>>>
>>>>> Tested with the following filters:
>>>>>
>>>>>    #tc qdisc add dev p4p1 ingress
>>>>>    #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>>>>      handle 800:0:1 u32 ht 800: \
>>>>>      match ip dst 11.0.0.1/24 match ip src 11.0.0.2/24 action drop
>>>>>
>>>>>    #tc filter del dev p4p1 parent ffff: protocol ip prio 99 \
>>>>>      handle 800:0:1 u32
>>>>>    #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>>>>      handle 1: u32 divisor 1
>>>>>    #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>>>>      handle 800:0:10 u32 ht 800: link 1: \
>>>>>      offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6 ff
>>>>>    #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>>>>      handle 1:0:10 u32 ht 1: \
>>>>>      match tcp src 1024 ffff match tcp dst 80 ffff action drop
>>>>>
>>>>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>>>>> ---
>>>> But this will break setting only dst port or only src port.
>>> No. This will not break specifying only src or dst port. The value/mask
>>> for the
>>> unspecified port will be set to zero. So it should be fine.
>>>
>>> For ex:
>>>      match tcp src 1024 ffff match tcp dst 80 ffff
>>>          => match 04000050/ffffffff at nexthdr+0
>>>      match tcp src 1024 ffff
>>>          => match 04000000/ffff0000 at nexthdr+0
>>>      match tcp dst 80 ffff
>>>          => match 00000050/0000ffff at nexthdr+0
>>>
>>>
>>>>    Do we
>>>> actually need three signatures to match? Something like,
>>>>
>>>>     static struct ixgbe_mat_field ixgbe_tcp_fields[] = {
>>>>      {.off = 0, .mask = 0xffffffff, .val = ixgbe_mat_prgm_ports,
>>>>          .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
>>>>      {.off = 0, .mask = 0xffff0000, .val = ixgbe_mat_prgm_dport,
>>>>          .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
>>>>      {.off = 0, .mask = 0x0000ffff, .val = ixgbe_mat_prgm_sport,
>>>>          .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
>>>>         { .val = NULL } /* terminal node */
>>>>     };
>>>>
>>>> Also just a reminder if we get multiple fields in a ixgbe_mat_field
>>>> struct we need to abort out of the for loop in the cls_u32 configure
>>>> function. Actually we can probably just push that as its own patch
>>>> to make the core function more versatile/usable.
>>>>
>>>> Thanks,
>>>> John
>> Dropped netdev no reason to push mail to netdev that has to do with our
>> driver internals.
>>
>> OK I see so you dropped the .mask check in fact you actually put it
>> there in the commit message I just missed the detail/implication.
>>
>> I think this is fine and it allows supporting mask entries now which
>> I blocked in the initial submission. But with this is there any reason
>> to have a mask field in ixgbe_mat_field? We could probably do this with
>> two patches, one to drop the 'mask' field and check and another to fix
>> the ixgbe_tcp_fields patch?
> 
> OK. will remove the 'mask' field and resubmit as 2 patches.
> 
> Thanks
> Sridhar

Great nice catch / improvements. By the way my goal here is to get
to the point where we can share this across drivers and just change
the *model.h file.

.John