[ovs-dev] [per-pmd ftb/cls 3/3] dpif-netdev: Add per-pmd flow-table/classifier.

[Gray, Mark D]

> 
> 	Thanks for this patch. It's good that some of these locks are
> 	being removed from the datapath. I reviewed the dpif-netdev.c
> 	file. There is an XXX in fast_path_processing() - Can that be
> 	resolved now as you have a per-core classifier or do I
> 	misunderstand? Other than that the code is clean and I didn't
> 	see any major issues.
> 
> 
> 
> 
> Thx for the feedback!~  For the 'XXX' part, I'll conduct more experiment to
> see if we could extend the critical section by moving mutex outside the loop.
> However, I'm a bit worried that it could affect the revalidator execution.

I just wanted to point it out. It may not need to be fixed as part of
this patch set and could be a separate patch. I might look at it myself.
> 
> We need the 'flow_mutex' because revalidators may delete flows and
> 
> users may manually add/del flows via 'ovs-appctl dpctl/*' commands.
> So, the extended critical section could lead to worse contention between
> pmd thread and revalidator threads.
> 
> 
> 
> 
> 	I did some tests with just single pmd and 30,000
> 	flows to see what kind of performance bump I could get with these
> 	patches and I saw about a 30% increase in throughput (i didn't look
> 	at latency). I guess even if there wasn't any contention on the stats
> 	mutexes there should be a bump in performance because of the
> 	overhead of locking the mute but I checked vtune and there was less
> 	contention on mutexes.
> 
> 
> 
> 
> I have never used vtune.  But from analyzing using perf, if there is no
> contention on the stats mutexes, the cpu util of locking/unlocking seems
> negligible, overshadowed by other operations from the pmd thread main
> loop.
> 
> 
vtune is similar to perf but has nice graphs :) It also has a "locksandwaits"
test which shows contention issues.
> 
> 
> 	The flow_mutex is another one that I have seen cause contention, I
> 	think this might be a bit trickier to remove. The way we did it in
> 	OVDK was to queue up requests to alter the flow table and then
> 	execute them when we had time (which was just intermittently).
> Have
> 	you thought about how to resolve this?
> 
> 
> 
> 
> One thing we've discussed around is to make pmd thread revalidate flows
> itself.  We'll experiment on that.  If it is implemented, we could then queue
> up
> user flow add/del operations and totally eliminate the flow mutex.  It is tricky
> 
> to do it efficiently, we will do more research on that,

I suppose there is a tradeoff which depends on how long validation takes. If
the pmd thread is doing revalidation, it could introduce packet latency. Maybe
there could be set preemption points in the revalidation that would yield
the thread to packet processing. Either way, it's a discussion for another
day.
> 
> 
> Do all these make sense?

Yes. 

Patch looks good.

Tested-by: Mark D. Gray <mark.d.gray at intel.com>

> 
> Thanks,
> Alex Wang,
>