[ovs-dev] [PATCH v4] dpif-netdev: dpcls per in_port with sorted subtables

[Daniele Di Proietto]

2016-08-11 1:48 GMT-07:00 Jan Scheurich <jan.scheurich at ericsson.com>:

> Hi Daniele,
>
>
>
> Thanks for the review. I will send v5 asap.
>
> A few answers inline below.
>
>
>
> BR, Jan
>
>
>
> *From:* Daniele Di Proietto [mailto:diproiettod at ovn.org]
> *Sent:* Thursday, 11 August, 2016 02:59
> *To:* Jan Scheurich
> *Cc:* dev at openvswitch.org; antonio.fischetti at intel.com
> *Subject:* Re: [ovs-dev] [PATCH v4] dpif-netdev: dpcls per in_port with
> sorted subtables
>
>
>
> Hi Jan,
>
> thanks for the patch!
>
> I tried benchmarking with flow tables from utilities/ovs-pipegen.py and
> the results look very good.
>
> [Jan] Do you have any numbers? Setting up pipelines is one thing. Is there
> are standard test setup for measuring DPDK datapath performance for these
> pipelines also? Seems non-trivial. Everyone have their own setup and
> figures are difficult to compare.
>
We don't have a standard testing.  This time I just used ovs-pipegen.py to
program the pipeline, and I used a sample packet capture (the packets are
truncated, to stress the classification more) from one of our cloud
environment.  Even if the packets don't match exactly the addresses in the
pipeline, you still get interesting megaflows.  This doesn't test the
per-port classifier, it just benefits from the subtable ordering.

Now that we have OVN gateway, we can start using that to generate even more
realistic use cases.  I think I'll start looking into that.

> I had to manually edit the patch to apply: some whitespaces are trimmed,
> some lines are truncated and the form feeds('^L') are two separate
> characters.  Would you mind using git send-email next time?
>
>
>
> [Jan] I will try to use git send-email. Never done it before. Please bear
> with me.
>

I received it without any problems this time, thanks


>
>
> I have a few minor comments inline
>
> Thanks,
>
>
>
> Daniele
>
>
>
>  static void
>  dp_netdev_pmd_remove_flow(struct dp_netdev_pmd_thread *pmd,
>                            struct dp_netdev_flow *flow)
>      OVS_REQUIRES(pmd->flow_mutex)
>  {
>      struct cmap_node *node = CONST_CAST(struct cmap_node *, &flow->node);
> +    struct dpcls *cls;
> +    odp_port_t in_port = flow->flow.in_port.odp_port;
>
> -    dpcls_remove(&pmd->cls, &flow->cr);
> +    cls = dp_netdev_pmd_lookup_dpcls(pmd, in_port);
> +    ovs_assert(cls != NULL);
> +    dpcls_remove(cls, &flow->cr);
>
>
>
> Do you think we should remove the dpcls if it becomes empty?
>
>
>
> [Jan] Jarno initially suggested the same and I considered but dropped the
> idea for the following reasons:
>
> a) The memory overhead of an empty dpcls is not significant
>
> b) Freed DP port numbers (and hence empty dpcls instances) are re-used. So
> even if e.g. dpdkvhostuser ports come and go as VMs are started and
> stopped, the set of dpcls instances will not grow over time.
>
>
Ok, I think it's fine for now.  On a related note, I discussed this with
Jarno and we think that maybe it's worth having a stricter relation with
struct dp_netdev_port.  As I said, this doesn't need to be done now, but
maybe we should investigate it in the future.


>
>
> +static inline void
> +dp_netdev_pmd_try_optimize(struct dp_netdev_pmd_thread *pmd)
> +{
> +    struct dpcls *cls;
> +    long long int now = time_msec();
> +
> +    if (now > pmd->next_optimization) {
> +        /* Try to obtain the flow lock to block out revalidator threads.
> +         * If not possible, just try next time. */
> +        if (ovs_mutex_trylock(&pmd->flow_mutex)) {
>
>
>
> ovs_mutex_trylock() returns non-zero if it fails, zero if it succeeds, we
> need an extra !
>
> This was reported by a clang warning, I think it's extremely useful to
> deal with concurrency.
>
>
>
> [Jan] Thanks for spotting this. It effectively disables sorting, which I
> didn’t notice as unexpectedly high number avg. subtable lookups because in
> my minimal performance test setup, there is just one subtable per in_port.
> Will try to have a go at clang also.
>
>
>