[ovs-dev] [PATCH RFC 1/1] netdev-dpdk.c: Add ingress-policing functionality.

[Daniele Di Proietto]

Hi Ian,

On 07/04/2016 06:00, "Stokes, Ian" <ian.stokes at intel.com> wrote:

>> > >71034a0..faf3583 100644
>> > >--- a/lib/netdev-dpdk.c
>> > >+++ b/lib/netdev-dpdk.c
>> > >@@ -53,6 +53,7 @@
>> > >
>> > > #include "rte_config.h"
>> > > #include "rte_mbuf.h"
>> > >+#include "rte_meter.h"
>> > > #include "rte_virtio_net.h"
>> > >
>> > > VLOG_DEFINE_THIS_MODULE(dpdk);
>> > >@@ -193,6 +194,11 @@ struct dpdk_ring {
>> > >     struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);  };
>> > >
>> > >+struct ingress_policer {
>> > >+    struct rte_meter_srtcm_params app_srtcm_params;
>> > >+    struct rte_meter_srtcm in_policer; };
>> > >+
>> > > struct netdev_dpdk {
>> > >     struct netdev up;
>> > >     int port_id;
>> > >@@ -231,6 +237,13 @@ struct netdev_dpdk {
>> > >     /* Identifier used to distinguish vhost devices from each other
>> */
>> > >     char vhost_id[PATH_MAX];
>> > >
>> > >+    /* Ingress Policer */
>> > >+    rte_spinlock_t policer_lock;
>> > >+    struct ingress_policer *ingress_policer;
>> > >+
>> >
>> > I would prefer not to have a lock at this level.
>> >
>> > I think it would make more sense to make the ingress_policer pointer
>> > RCU protected and embed the spinlock into struct ingress_policer, to
>> > protect the token bucket.
>> >
>> Sure I agree, I was modelling this on what we have currently in QoS just
>> for a rough implementation.
>> I can take a look at using RCU instead. This could possibly be extended
>> to the QoS use case also in the futue.
>
>Hi Daniele, I have been looking at an RCU implementation here but so far
>have not been able to get it working correctly.
>
>The issue I'm seeing is when I destroy the policer while traffic is
>passing a segfault sometimes occurs as the meter is in use when the
>ingress policer is set to NULL.
>
>I'm pretty sure this is down to my understanding (or lack thereof) of the
>ovsrcu behavior.
>
>Is the following high level implementation correct in your eyes?
>
>The ingress policer struct is as follows:
>
>struct ingress_policer {
>    struct rte_meter_srtcm_params app_srtcm_params;
>    struct rte_meter_srtcm in_policer;
>};
>
>From your comment above you mention embedding the spinlock in the ingress
>policer struct.
>Just to clarify, does the rcu by nature embed a spinlock or did you mean
>move the rte_spinlock policer_lock from the netdev_dpdk struct into the
>ingress policer struct?
>
>Is the behavior you are thinking of something like the following for when
>traffic is being processed?
>
>1. Get the rcu ingress_policer pointer.
>2. Lock the spinlock in the ingress policer struct.
>3. Set the ovsrcu pointer
>4. Call ovsrcu_synchronize to that all threads see that the policer is
>locked (Stop threads from accessing the ingress policer)
>5. Process the packets in the meter as usual.
>6. Unlock the spinlock.
>7. Set the ovsrcu pointer
>6. Synchronize again? (So that threads can access the ingress policer
>again)
>
>For destroying the ingress policer
>
>1. Get the rcu ingress_policer pointer.
>2. Lock the spinlock in the ingress policer struct.
>3. Set the ovsrcu pointer
>4. Call ovsrcu_synchronize to that all threads see that the policer is
>locked (Stop threads from accessing the meter)
>5. Destroy the ingress policer.
>6. Unlock the spinlock - if the spinlock is embedded in the ingress
>policer struct we have a problem here as it cannot  be free now, the
>struct has been destroyed.
>7. Set the ovsrcu pointer
>8. Synchronize again? (So that threads can see the ingress policer point
>for the netdev is now null)
>
>Thanks
>Ian
>

What I had in mind was simpler:

processing traffic:

p = ovsrcu_get(&dev->ingress_policer)
if (p) {
    rte_spinlock_lock(&p->lock);
    policer_pkt_handle(p, pkts...);
    rte_spinlock_unlock(&p->unlock);
}

destroying:

ovs_mutex_lock(&dev->mutex);
...
p = ovsrcu_get_protected(&dev->ingress_policer);
ovsrcu_postpone(destroy_policer, p);
ovsrcu_set(&dev->ingress_policer, NULL);
...
ovs_mutex_unlock(&dev->mutex);


static void destroy_policer (struct ingress_policer *p)
{
    /*...*/
    free(p);
}

My goal was to avoid taking the spinlock unless QoS in configured (p !=
NULL).


The pointer returned by ovsrcu_get() is guaranteed to be valid until the
next grace period, because ovsrcu_postpone() will not call free() until
the next grace period.

Or, from another point of view, after a grace period, when
ovsrcu_postpone() will actually call free(), all the other threads must
see the new value (NULL), so it is safe to reclaim memory.

The spinlock is used just to protect the meter.

Does this make sense?

Hope this helps,

Daniele