Re: [oss-drivers] Re: [PATCH/RFC net-next 7/9] nfp: add metadata to each flow offload

2017-06-28 Thread Simon Horman 
On Tue, Jun 27, 2017 at 11:15:20PM -0700, Jakub Kicinski wrote:
> On Wed, 28 Jun 2017 01:21:47 +0200, Simon Horman wrote:
> > From: Pieter Jansen van Vuuren 
> > 
> > Adds metadata describing the mask id of each flow and keeps track of
> > flows installed in hardware. Previously a flow could not be removed
> > from hardware as there was no way of knowing if that a specific flow
> > was installed. This is solved by storing the offloaded flows in a
> > hash table.
t reb> > 
> > Signed-off-by: Pieter Jansen van Vuuren 
> > 
> > Signed-off-by: Simon Horman 
> 
> > diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.h 
> > b/drivers/net/ethernet/netronome/nfp/flower/main.h
> > index 52db2acb250e..cc184618306c 100644
> > --- a/drivers/net/ethernet/netronome/nfp/flower/main.h
> > +++ b/drivers/net/ethernet/netronome/nfp/flower/main.h
> > @@ -34,6 +34,7 @@
> >  #ifndef __NFP_FLOWER_H__
> >  #define __NFP_FLOWER_H__ 1
> >  
> > +#include 
> >  #include 
> >  
> >  #include "cmsg.h"
> > @@ -45,6 +46,42 @@ struct tc_to_netdev;
> >  struct net_device;
> >  struct nfp_app;
> >  
> > +#define NFP_FLOWER_HASH_BITS   10
> > +#define NFP_FLOWER_HASH_SEED   129004
> > +
> > +#define NFP_FLOWER_MASK_ENTRY_RS   256
> > +#define NFP_FLOWER_MASK_ELEMENT_RS 1
> > +#define NFP_FLOWER_MASK_HASH_BITS  10
> > +#define NFP_FLOWER_MASK_HASH_SEED  9198806
> > +
> > +#define NFP_FL_META_FLAG_NEW_MASK  128
> > +#define NFP_FL_META_FLAG_LAST_MASK 1
> > +
> > +#define NFP_FL_MASK_REUSE_TIME 40
> > +#define NFP_FL_MASK_ID_LOCATION1
> > +
> > +struct nfp_fl_mask_id {
> > +   struct circ_buf mask_id_free_list;
> > +   struct timeval *last_used;
> > +   u8 init_unallocated;
> > +};
> > +
> > +/**
> > + * struct nfp_flower_priv - Flower APP per-vNIC priv data
> > + * @nn:Pointer to vNIC
> > + * @flower_version:HW version of flower
> > + * @mask_ids:  List of free mask ids
> > + * @mask_table:Hash table used to store masks
> > + * @flow_table:Hash table used to store flower rules
> > + */
> > +struct nfp_flower_priv {
> > +   struct nfp_net *nn;
> > +   u64 flower_version;
> > +   struct nfp_fl_mask_id mask_ids;
> > +   DECLARE_HASHTABLE(mask_table, NFP_FLOWER_MASK_HASH_BITS);
> > +   DECLARE_HASHTABLE(flow_table, NFP_FLOWER_HASH_BITS);
> 
> Include for hashtable seems missing.

Thanks, I'll include linux/hashtable.h

> > +};
> > +
> >  struct nfp_fl_key_ls {
> > u32 key_layer_two;
> > u8 key_layer;
> > @@ -69,6 +106,10 @@ struct nfp_fl_payload {
> > char *action_data;
> >  };
> >  
> > +int nfp_flower_metadata_init(struct nfp_app *app);
> > +void nfp_flower_metadata_cleanup(struct nfp_app *app);
> > +
> > +int nfp_repr_get_port_id(struct net_device *netdev);
> 
> Isn't this a static inline in repr.h?

Sorry, I think that crept in during some patch shuffling.
I will remove it.

> >  int nfp_flower_repr_init(struct nfp_app *app);
> >  int nfp_flower_setup_tc(struct nfp_app *app, struct net_device *netdev,
> > u32 handle, __be16 proto, struct tc_to_netdev *tc);
> 
> > diff --git a/drivers/net/ethernet/netronome/nfp/flower/metadata.c 
> > b/drivers/net/ethernet/netronome/nfp/flower/metadata.c
> > new file mode 100644
> > index ..acbf4c757988
> 
> > + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> > + * SOFTWARE.
> > + */
> > +
> > +#include 
> 
> I think this is unnecessary.

Thanks, removed.

> 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> 
> > +static int nfp_mask_alloc(struct nfp_app *app, u8 *mask_id)
> > +{
> > +   struct nfp_flower_priv *priv = app->priv;
> > +   struct timeval reuse, curr;
> > +   struct circ_buf *ring;
> > +   u8 temp_id, freed_id;
> > +
> > +   ring = >mask_ids.mask_id_free_list;
> > +   freed_id = NFP_FLOWER_MASK_ENTRY_RS - 1;
> > +   /* Checking for unallocated entries first. */
> > +   if (priv->mask_ids.init_unallocated > 0) {
> > +   *mask_id = priv->mask_ids.init_unallocated;
> > +   priv->mask_ids.init_unallocated--;
> > +   goto exit_check_timestamp;
> 
> Do you really need to check the timestamp here?  Isn't this if() for the
> case where we have some masks which were never used by the driver?

I think not. I will drop this.

> > +   }
> > +
> > +   /* Checking if buffer is empty. */
> > +   if (ring->head == ring->tail) {
> > +   *mask_id = freed_id;
> > +   return -ENOENT;
> > +   }
> > +
> > +   memcpy(_id, >buf[ring->tail], NFP_FLOWER_MASK_ELEMENT_RS);
> > +   *mask_id = temp_id;
> > +   memcpy(>buf[ring->tail], _id, NFP_FLOWER_MASK_ELEMENT_RS);
> > +
> > +   ring->tail = (ring->tail + NFP_FLOWER_MASK_ELEMENT_RS) %
> > +(NFP_FLOWER_MASK_ENTRY_RS * NFP_FLOWER_MASK_ELEMENT_RS);
> > +
> > +exit_check_timestamp:
> > +   do_gettimeofday();
> >

Re: [PATCH/RFC net-next 7/9] nfp: add metadata to each flow offload

2017-06-28 Thread Jakub Kicinski 
On Wed, 28 Jun 2017 01:21:47 +0200, Simon Horman wrote:
> From: Pieter Jansen van Vuuren 
> 
> Adds metadata describing the mask id of each flow and keeps track of
> flows installed in hardware. Previously a flow could not be removed
> from hardware as there was no way of knowing if that a specific flow
> was installed. This is solved by storing the offloaded flows in a
> hash table.
> 
> Signed-off-by: Pieter Jansen van Vuuren 
> Signed-off-by: Simon Horman 

> diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.h 
> b/drivers/net/ethernet/netronome/nfp/flower/main.h
> index 52db2acb250e..cc184618306c 100644
> --- a/drivers/net/ethernet/netronome/nfp/flower/main.h
> +++ b/drivers/net/ethernet/netronome/nfp/flower/main.h
> @@ -34,6 +34,7 @@
>  #ifndef __NFP_FLOWER_H__
>  #define __NFP_FLOWER_H__ 1
>  
> +#include 
>  #include 
>  
>  #include "cmsg.h"
> @@ -45,6 +46,42 @@ struct tc_to_netdev;
>  struct net_device;
>  struct nfp_app;
>  
> +#define NFP_FLOWER_HASH_BITS 10
> +#define NFP_FLOWER_HASH_SEED 129004
> +
> +#define NFP_FLOWER_MASK_ENTRY_RS 256
> +#define NFP_FLOWER_MASK_ELEMENT_RS   1
> +#define NFP_FLOWER_MASK_HASH_BITS10
> +#define NFP_FLOWER_MASK_HASH_SEED9198806
> +
> +#define NFP_FL_META_FLAG_NEW_MASK128
> +#define NFP_FL_META_FLAG_LAST_MASK   1
> +
> +#define NFP_FL_MASK_REUSE_TIME   40
> +#define NFP_FL_MASK_ID_LOCATION  1
> +
> +struct nfp_fl_mask_id {
> + struct circ_buf mask_id_free_list;
> + struct timeval *last_used;
> + u8 init_unallocated;
> +};
> +
> +/**
> + * struct nfp_flower_priv - Flower APP per-vNIC priv data
> + * @nn:  Pointer to vNIC
> + * @flower_version:  HW version of flower
> + * @mask_ids:List of free mask ids
> + * @mask_table:  Hash table used to store masks
> + * @flow_table:  Hash table used to store flower rules
> + */
> +struct nfp_flower_priv {
> + struct nfp_net *nn;
> + u64 flower_version;
> + struct nfp_fl_mask_id mask_ids;
> + DECLARE_HASHTABLE(mask_table, NFP_FLOWER_MASK_HASH_BITS);
> + DECLARE_HASHTABLE(flow_table, NFP_FLOWER_HASH_BITS);

Include for hashtable seems missing.

> +};
> +
>  struct nfp_fl_key_ls {
>   u32 key_layer_two;
>   u8 key_layer;
> @@ -69,6 +106,10 @@ struct nfp_fl_payload {
>   char *action_data;
>  };
>  
> +int nfp_flower_metadata_init(struct nfp_app *app);
> +void nfp_flower_metadata_cleanup(struct nfp_app *app);
> +
> +int nfp_repr_get_port_id(struct net_device *netdev);

Isn't this a static inline in repr.h?

>  int nfp_flower_repr_init(struct nfp_app *app);
>  int nfp_flower_setup_tc(struct nfp_app *app, struct net_device *netdev,
>   u32 handle, __be16 proto, struct tc_to_netdev *tc);

> diff --git a/drivers/net/ethernet/netronome/nfp/flower/metadata.c 
> b/drivers/net/ethernet/netronome/nfp/flower/metadata.c
> new file mode 100644
> index ..acbf4c757988

> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#include 

I think this is unnecessary.

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

> +static int nfp_mask_alloc(struct nfp_app *app, u8 *mask_id)
> +{
> + struct nfp_flower_priv *priv = app->priv;
> + struct timeval reuse, curr;
> + struct circ_buf *ring;
> + u8 temp_id, freed_id;
> +
> + ring = >mask_ids.mask_id_free_list;
> + freed_id = NFP_FLOWER_MASK_ENTRY_RS - 1;
> + /* Checking for unallocated entries first. */
> + if (priv->mask_ids.init_unallocated > 0) {
> + *mask_id = priv->mask_ids.init_unallocated;
> + priv->mask_ids.init_unallocated--;
> + goto exit_check_timestamp;

Do you really need to check the timestamp here?  Isn't this if() for the
case where we have some masks which were never used by the driver?

> + }
> +
> + /* Checking if buffer is empty. */
> + if (ring->head == ring->tail) {
> + *mask_id = freed_id;
> + return -ENOENT;
> + }
> +
> + memcpy(_id, >buf[ring->tail], NFP_FLOWER_MASK_ELEMENT_RS);
> + *mask_id = temp_id;
> + memcpy(>buf[ring->tail], _id, NFP_FLOWER_MASK_ELEMENT_RS);
> +
> + ring->tail = (ring->tail + NFP_FLOWER_MASK_ELEMENT_RS) %
> +  (NFP_FLOWER_MASK_ENTRY_RS * NFP_FLOWER_MASK_ELEMENT_RS);
> +
> +exit_check_timestamp:
> + do_gettimeofday();
> + reuse.tv_sec = curr.tv_sec -
> +priv->mask_ids.last_used[*mask_id].tv_sec;
> + reuse.tv_usec = curr.tv_usec -
> + priv->mask_ids.last_used[*mask_id].tv_usec;

Could you perhaps use timespec64 as the type?  You will then be able to
use timespec64_sub() and it will save a nsec -> usec conversion in
do_gettimeofday().

> + if (!reuse.tv_sec && reuse.tv_usec < NFP_FL_MASK_REUSE_TIME) {
>

[PATCH/RFC net-next 7/9] nfp: add metadata to each flow offload

2017-06-27 Thread Simon Horman 
From: Pieter Jansen van Vuuren 

Adds metadata describing the mask id of each flow and keeps track of
flows installed in hardware. Previously a flow could not be removed
from hardware as there was no way of knowing if that a specific flow
was installed. This is solved by storing the offloaded flows in a
hash table.

Signed-off-by: Pieter Jansen van Vuuren 
Signed-off-by: Simon Horman 
---
 drivers/net/ethernet/netronome/nfp/Makefile|   1 +
 drivers/net/ethernet/netronome/nfp/flower/main.c   |   8 -
 drivers/net/ethernet/netronome/nfp/flower/main.h   |  52 +++
 .../net/ethernet/netronome/nfp/flower/metadata.c   | 379 +
 .../net/ethernet/netronome/nfp/flower/offload.c|  30 +-
 5 files changed, 459 insertions(+), 11 deletions(-)
 create mode 100644 drivers/net/ethernet/netronome/nfp/flower/metadata.c

diff --git a/drivers/net/ethernet/netronome/nfp/Makefile 
b/drivers/net/ethernet/netronome/nfp/Makefile
index 1ba0ea78adc3..b8e1358868bd 100644
--- a/drivers/net/ethernet/netronome/nfp/Makefile
+++ b/drivers/net/ethernet/netronome/nfp/Makefile
@@ -35,6 +35,7 @@ nfp-objs += \
flower/cmsg.o \
flower/main.o \
flower/match.o \
+   flower/metadata.o \
flower/offload.o
 endif
 
diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.c 
b/drivers/net/ethernet/netronome/nfp/flower/main.c
index 7b27871f489c..a7c9dea8cb9c 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.c
@@ -47,14 +47,6 @@
 #include "../nfp_port.h"
 #include "./cmsg.h"
 
-/**
- * struct nfp_flower_priv - Flower APP per-vNIC priv data
- * @nn: Pointer to vNIC
- */
-struct nfp_flower_priv {
-   struct nfp_net *nn;
-};
-
 static const char *nfp_flower_extra_cap(struct nfp_app *app, struct nfp_net 
*nn)
 {
return "FLOWER";
diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.h 
b/drivers/net/ethernet/netronome/nfp/flower/main.h
index 52db2acb250e..cc184618306c 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.h
@@ -34,6 +34,7 @@
 #ifndef __NFP_FLOWER_H__
 #define __NFP_FLOWER_H__ 1
 
+#include 
 #include 
 
 #include "cmsg.h"
@@ -45,6 +46,42 @@ struct tc_to_netdev;
 struct net_device;
 struct nfp_app;
 
+#define NFP_FLOWER_HASH_BITS   10
+#define NFP_FLOWER_HASH_SEED   129004
+
+#define NFP_FLOWER_MASK_ENTRY_RS   256
+#define NFP_FLOWER_MASK_ELEMENT_RS 1
+#define NFP_FLOWER_MASK_HASH_BITS  10
+#define NFP_FLOWER_MASK_HASH_SEED  9198806
+
+#define NFP_FL_META_FLAG_NEW_MASK  128
+#define NFP_FL_META_FLAG_LAST_MASK 1
+
+#define NFP_FL_MASK_REUSE_TIME 40
+#define NFP_FL_MASK_ID_LOCATION1
+
+struct nfp_fl_mask_id {
+   struct circ_buf mask_id_free_list;
+   struct timeval *last_used;
+   u8 init_unallocated;
+};
+
+/**
+ * struct nfp_flower_priv - Flower APP per-vNIC priv data
+ * @nn:Pointer to vNIC
+ * @flower_version:HW version of flower
+ * @mask_ids:  List of free mask ids
+ * @mask_table:Hash table used to store masks
+ * @flow_table:Hash table used to store flower rules
+ */
+struct nfp_flower_priv {
+   struct nfp_net *nn;
+   u64 flower_version;
+   struct nfp_fl_mask_id mask_ids;
+   DECLARE_HASHTABLE(mask_table, NFP_FLOWER_MASK_HASH_BITS);
+   DECLARE_HASHTABLE(flow_table, NFP_FLOWER_HASH_BITS);
+};
+
 struct nfp_fl_key_ls {
u32 key_layer_two;
u8 key_layer;
@@ -69,6 +106,10 @@ struct nfp_fl_payload {
char *action_data;
 };
 
+int nfp_flower_metadata_init(struct nfp_app *app);
+void nfp_flower_metadata_cleanup(struct nfp_app *app);
+
+int nfp_repr_get_port_id(struct net_device *netdev);
 int nfp_flower_repr_init(struct nfp_app *app);
 int nfp_flower_setup_tc(struct nfp_app *app, struct net_device *netdev,
u32 handle, __be16 proto, struct tc_to_netdev *tc);
@@ -79,5 +120,16 @@ int nfp_flower_compile_flow_match(struct 
tc_cls_flower_offload *flow,
 int nfp_flower_compile_action(struct tc_cls_flower_offload *flow,
  struct net_device *netdev,
  struct nfp_fl_payload *nfp_flow);
+int nfp_compile_flow_metadata(struct nfp_app *app,
+ struct tc_cls_flower_offload *flow,
+ struct nfp_fl_payload *nfp_flow);
+int nfp_modify_flow_metadata(struct nfp_app *app,
+struct nfp_fl_payload *nfp_flow);
+
+struct nfp_fl_payload *
+nfp_flower_find_in_fl_table(struct nfp_app *app,
+   unsigned long tc_flower_cookie);
+int nfp_flower_remove_fl_table(struct nfp_app *app,
+  unsigned long tc_flower_cookie);
 
 #endif
diff