Hey Pravin,
Thx for the review, please see my comments below,
> + if (poller_id != f.poller_id) {
> > + if (f.poller_id == NON_PMD_CORE_ID) {
> > + ds_put_format(&ds, "flow-dump from non-dpdk
> interfaces:\n");
> > + } else {
> > + ds_put_format(&ds, "flow-dump from pmd on cpu core:
> %d\n",
> > + f.poller_id);
> > + }
> > + poller_id = f.poller_id;
> > + }
> > +
>
> You have introduced new 'poller' term. It is right term here but we
> already have pmd used in same context. I would like to use same rather
> than introducing another name for same.
>
>
Sure, I'll just use the term 'pmd_id' everywhere.
> +
> > + numa_id = netdev_get_numa_id(in_port_netdev);
> > + if (ovs_numa_numa_id_is_valid(numa_id)) {
> > + unsigned long *bm;
> > + int n_cores = ovs_numa_get_n_cores();
> > + int idx;
> > +
> > + bm = bitmap_allocate(n_cores);
> > + ovs_numa_get_pinned_cores_on_numa(numa_id, bm);
> > +
> > + BITMAP_FOR_EACH_1 (idx, n_cores, bm) {
>
> Rather than exporting ovs_numa_get_pinned_cores_on_numa() API, we can
> export
> iterator API for_each_cpu_on_numa() that ierates cpu from given NUMA node.
>
This is a bit tricky, since 'struct numa' and 'struct core' are not visible
from outside the module.
I'm planning to add these two structs for exposing core_id, numa_id pairs:
struct ovs_numa_dump {
struct list dump;
};
struct ovs_numa_info {
struct list list_node;
int numa_id;
int core_id;
};
And provide the corresponding dump functions.
> > int error = 0;
> >
> > - netdev_flow = dp_netdev_find_flow(dp, get->ufid, get->key,
> get->key_len);
> > + /* Each call must provide valid poller_id. */
> > + if (!ovs_numa_core_id_is_valid(poller_id)) {
> > + return EINVAL;
> > + }
> > +
> do we really need to check ovs_numa_core_id_is_valid()?
> dp_netdev_get_pmd() will fail if it is invalid core anyways.
>
Thanks for pointing it out, I'll remove the check,
> >
> > +static void
> > +clear_stats(struct dp_netdev_flow *netdev_flow)
> > +{
> > + memset(&netdev_flow->stats, 0, sizeof netdev_flow->stats);
> > +}
> > +
> I guess this can be inlined.
done
> > dp_netdev_get_pmd(struct dp_netdev *dp, int core_id)
> > {
> > @@ -2454,10 +2499,12 @@ dp_netdev_get_pmd(struct dp_netdev *dp, int
> core_id)
> > const struct cmap_node *pnode;
> >
> > pnode = cmap_find(&dp->poll_threads, hash_int(core_id, 0));
> > - ovs_assert(pnode);
> > + if (!pnode) {
> > + return NULL;
> > + }
> > pmd = CONTAINER_OF(pnode, struct dp_netdev_pmd_thread, node);
> >
> > - return pmd;
> > + return dp_netdev_pmd_try_ref(pmd) ? pmd : NULL;
> > }
> Since pnode is already checked for NULL, is it even possible to return
> false there?
>
Yes, main thread could be removing the pmd thread while other threads are
executing between these lines.
Also after giving it a second thought, I think the dp_netdev_pmd_unref()
can not directly destroy the 'pmd' when ref_count reaches zero, since the
other threads may trying to count the 'pmd->ref_count' at same time.
So, I'll use ovsrcu_postpone() to delay the destroy.
>
> >
> > /* Sets the 'struct dp_netdev_pmd_thread' for non-pmd threads. */
> > @@ -2471,6 +2518,53 @@ dp_netdev_set_nonpmd(struct dp_netdev *dp)
> > OVS_NUMA_UNSPEC);
> > }
> >
> > +static bool
> > +dp_netdev_pmd_try_ref(struct dp_netdev_pmd_thread *pmd)
> > +{
> > + if (pmd) {
> > + return ovs_refcount_try_ref_rcu(&pmd->ref_cnt);
> > + }
> > +
> We can just drop this check since all callers make sure that they have
> valid pmd pointer
Yes, I'll drop it and add a comment.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
