Re: [patch net-next v2 00/10] introduce rocker switch driver with hardware accelerated datapath api - phase 1: bridge fdb offload

[Simon Horman <simon.horman@netronome.com>]

Hi John,

On Wed, Nov 12, 2014 at 10:31:06PM -0800, John Fastabend wrote:
> On 11/12/2014 09:44 PM, Simon Horman wrote:
> > [snip]
> > 
> >> Simon, if your feeling adventurous any feedback on the repo link
> >> would be great. I still need to smash the commit log into something
> >> coherent though at the moment you can see all the errors and rewrites,
> >> etc as I made them.
> > 
> > Hi John,
> > 
> > here is some preliminary feedback:
> > 
> > * I notice that the parse graph code isn't present yet.
> >   I suppose this is a difficult piece that naturally follows many
> >   other piece. None the less it is possibly the piece of most
> >   interest to me :-)
> 
> I can add this over the next few days. Also I wanted to publish some
> more complex examples on top of rocker switch. The nic drivers are
> interesting but not as complex as some of the switch devices.

I see that you have added the header graph, which seems pretty nice
from my reading so far.

It seems to allow for arbitrary connections between instances
of net_flow_header_node, including I loops I suppose. This seems
nice and flexible to me.

I have a very minor update to contribute which helped me to
read the code. Please feel free to squash/ignore/...

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_pipeline.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_pipeline.h
index a4818ab..4025a61 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_pipeline.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_pipeline.h
@@ -412,7 +412,7 @@ struct net_flow_jump_table ixgbe_vlan_inner_jump[2] = {
 		   .type = NET_FLOW_FIELD_REF_ATTR_TYPE_U16,
 		   .value_u16 = 0x0800,
 		},
-		.node = 4,
+		.node = HEADER_INSTANCE_IP,
 	},
 	{
 		.field = {0},
@@ -443,7 +443,7 @@ struct net_flow_jump_table ixgbe_ip_jump[2] = {
 		   .type = NET_FLOW_FIELD_REF_ATTR_TYPE_U8,
 		   .value_u8 = 0x06,
 		},
-		.node = 5,
+		.node = HEADER_INSTANCE_TCP,
 	},
 	{
 		.field = {0},

> There is also the table graph layout which I wanted tweak a bit. At
> the moment I have hardware that can run tables in parallel and some
> that executes tables in sequence. It might not be clear from the code
> (why I need the cleanup) but the source id is being used to indicate
> if the tables are executed in parallel or not.

Thanks, that was not clear to me.

> > * Will del and update flows require flows to already exist?
> >   And similarly, will add flow require flows with the same match to not
> >   already exist?  If so, the error handling seems tricky of more than one
> >   flow is to be deleted/updated. IIRC there was some discussion of that
> >   kind of issue at the (double) round table discussion on the last day of
> >   LPC14 in Düsseldorf.
> 
> I would expect del/updates for flows that don't exist should fail.

Am I right in thinking that del and update flow NDOs may take
a list of flows? If so I think some consideration needs
to be made for handling failure of e.g. the last element of
the list when the previous elements succeeded.

I suppose that user-space could dump the flow table if an error and
adjust its state accordingly. But that seems somewhat onerous.

> I didn't intend to add any checks in the kernel to verify the matches
> are unique. My opinion on this is that user space shouldn't add new
> duplicate flows. And if it does hardware resources will be wasted.

I don't have any strong opinions on that at this time.
But it does seem reasonable so long as its clear that is the case.

> > * Should the .node_count value of ixgbe_table_node_l2 be 3?
> >   ixgbe_table_graph_nodes has three elements but perhaps you
> >   are intentionally excluding the last element ixgbe_table_node_nil?
> > 
> 
> Actually I could just drop the node_count at the moment because I've
> been null terminating the arrays with null items.
> 
> I should either add a count field to all the structures or null terminate
> the arrays. For now I mostly null terminate the arrays when I use
> them. For example matches is null terminates same for actions.

It looks like you have move towards the null termination option.
No objections here.