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

[Stokes, Ian]

> -----Original Message-----
> From: Daniele Di Proietto [mailto:diproiettod at vmware.com]
> Sent: Friday, April 08, 2016 4:44 AM
> To: Stokes, Ian
> Cc: dev at openvswitch.org
> Subject: Re: [ovs-dev] [PATCH RFC 1/1] netdev-dpdk.c: Add ingress-
> policing functionality.
> 
> 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.
> 
Thanks Daniele, I understand now and this clears up the issue I was having.

Thanks
Ian
> The spinlock is used just to protect the meter.
> 
> Does this make sense?
> 
> Hope this helps,
> 
> Daniele