> On 05/23/2018 12:53 PM, Stokes, Ian wrote:
> >>> rte_mempool_ops_get_count is not exported by DPDK so it means it
> >>> cannot be used by OVS when using DPDK as a shared library.
> >>>
> >>> It was only being used for extra paranoid, things might change in
> >>> the future mode anyway, so remove it and just use rte_mempool_full.
> >>
> >> How about keeping the dpdk_mp_full() function in a following manner:
> >>
> >> static int
> >> dpdk_mp_full(const struct rte_mempool *mp) OVS_REQUIRES(dpdk_mp_mutex)
> {
> >> /* XXX: A comment about possible false positives and why we're not
> >> * worrying about them so much. */
> >> return rte_mempool_full(mp);
> >> }
> >>
> >> ?
> >>
> >> I feel that we still need some explanation inside the code.
> >> What do you think?
> >>
> >
> > It probably makes sense so that others unfamiliar will have context in
> the future.
> >
>
> Yes, it's a reasonable thing to do.
>
> > I've validated the other patches and was about to push them but I can
> hold off for this change instead. It's a minor change so I can push it as
> an incremental on the current set if it helps move the process along.
> >
>
> I'll send a new set, as I think it's easier to go from the previous code
> than add back in the function in an incrementals. As you've validated the
> other patches already, I think code review should be enough as it's a very
> minor change.
>
+1
> Kevin.
>
> > Ian
> >
> >> Best regards, Ilya Maximets.
> >>
> >>>
> >>> The mempools are still removed later and are checked to see that the
> >>> buffers are back in it. Previously the mempools were removed
> >>> straight away and there was no checking.
> >>>
> >>> Fixes: 91fccdad72a2 ("netdev-dpdk: Free mempool only when no in-use
> >>> mbufs.")
> >>> Reported-by: Timothy Redaelli <tredaelli at redhat.com>
> >>> Reported-by: Markos Chandras <mchandras at suse.de>
> >>> Signed-off-by: Kevin Traynor <ktraynor at redhat.com>
> >>> ---
> >>> lib/netdev-dpdk.c | 22 ++--------------------
> >>> 1 file changed, 2 insertions(+), 20 deletions(-)
> >>>
> >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> >>> 87152a7..ede1e5c 100644
> >>> --- a/lib/netdev-dpdk.c
> >>> +++ b/lib/netdev-dpdk.c
> >>> @@ -527,22 +527,4 @@ ovs_rte_pktmbuf_init(struct rte_mempool *mp
> >>> OVS_UNUSED, }
> >>>
> >>> -static int
> >>> -dpdk_mp_full(const struct rte_mempool *mp)
> >>> OVS_REQUIRES(dpdk_mp_mutex) -{
> >>> - unsigned ring_count;
> >>> - /* This logic is needed because rte_mempool_full() is not
> >> guaranteed to
> >>> - * be atomic and mbufs could be moved from mempool cache -->
> >> mempool ring
> >>> - * during the call. However, as no mbufs will be taken from the
> >> mempool
> >>> - * at this time, we can work around it by also checking the ring
> >> entries
> >>> - * separately and ensuring that they have not changed.
> >>> - */
> >>> - ring_count = rte_mempool_ops_get_count(mp);
> >>> - if (rte_mempool_full(mp) && rte_mempool_ops_get_count(mp) ==
> >> ring_count) {
> >>> - return 1;
> >>> - }
> >>> -
> >>> - return 0;
> >>> -}
> >>> -
> >>> /* Free unused mempools. */
> >>> static void
> >>> @@ -553,5 +535,5 @@ dpdk_mp_sweep(void)
> >>> ovs_mutex_lock(&dpdk_mp_mutex);
> >>> LIST_FOR_EACH_SAFE (dmp, next, list_node, &dpdk_mp_free_list) {
> >>> - if (dpdk_mp_full(dmp->mp)) {
> >>> + if (rte_mempool_full(dmp->mp)) {
> >>> VLOG_DBG("Freeing mempool \"%s\"", dmp->mp->name);
> >>> ovs_list_remove(&dmp->list_node); @@ -666,5 +648,5 @@
> >>> dpdk_mp_release(struct rte_mempool *mp)
> >>>
> >>> ovs_mutex_lock(&dpdk_mp_mutex);
> >>> - if (dpdk_mp_full(mp)) {
> >>> + if (rte_mempool_full(mp)) {
> >>> VLOG_DBG("Freeing mempool \"%s\"", mp->name);
> >>> rte_mempool_free(mp);
> >>> --
> >>> 1.8.3.1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
