> -----Original Message----- > From: Daniele Di Proietto [mailto:diproiet...@vmware.com] > Sent: Friday, April 08, 2016 4:44 AM > To: Stokes, Ian > Cc: dev@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.sto...@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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
