| Message ID | 1527069360-14033-1-git-send-email-ktraynor@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ian Stokes |
| Headers | show |
| Series | [ovs-dev] netdev-dpdk: Remove use of rte_mempool_ops_get_count. | expand |
> 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? 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
> > 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. 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. 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
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. 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
> 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
On 05/23/2018 01:05 PM, Stokes, Ian wrote: >> 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? >>>> Thanks for suggestion. Added in v2. The nicest thing about this suggestion is that the patch now applies to the other branches :-) >>> >>> 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 @Ian, just to re-iterate the above, the v2 patch applies cleanly on branches 2.6/2.7/2.8/2.9, thanks, Kevin. > >> 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 >
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);
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. 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@redhat.com> Reported-by: Markos Chandras <mchandras@suse.de> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> --- lib/netdev-dpdk.c | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-)