[ovs-dev] [PATCH v8 4/6] netdev-dpdk: manage failure in mempool name creation.

[Darrell Ball]

On 10/19/17, 9:56 AM, "ovs-dev-bounces at openvswitch.org on behalf of antonio.fischetti at intel.com" <ovs-dev-bounces at openvswitch.org on behalf of antonio.fischetti at intel.com> wrote:

    In case a mempool name could not be generated log a message
    and return a null mempool pointer to the caller.
    
    CC: Mark B Kavanagh <mark.b.kavanagh at intel.com>
    CC: Darrell Ball <dlu998 at gmail.com>
    CC: Ciara Loftus <ciara.loftus at intel.com>
    CC: Kevin Traynor <ktraynor at redhat.com>
    CC: Aaron Conole <aconole at redhat.com>
    Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for each port.")
    Signed-off-by: Antonio Fischetti <antonio.fischetti at intel.com>
    ---
     lib/netdev-dpdk.c | 7 +++++++
     1 file changed, 7 insertions(+)
    
    diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
    index dc1e9c3..6fc6e1b 100644
    --- a/lib/netdev-dpdk.c
    +++ b/lib/netdev-dpdk.c
    @@ -502,6 +502,9 @@ dpdk_mp_name(struct dpdk_mp *dmp)
         int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, "ovs_%x_%d_%d_%u",
                            h, dmp->socket_id, dmp->mtu, dmp->mp_size);
         if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {

I see you copied me on an earlier version, so I’ll add one comment.
Given MTU size restriction for dpdk and rte max queues and max buf desc enforcement, can this condition be met ?
I think there are 11 remaining characters (out of 31) for mp_size and the calculation is 

dmp->mp_size = dev->requested_n_rxq * dev->requested_rxq_size
            + dev->requested_n_txq * dev->requested_txq_size
            + MIN(RTE_MAX_LCORE, dev->requested_n_rxq) * NETDEV_MAX_BURST
            + MIN_NB_MBUF;

I got 9 decimal digits max, assuming my math is correct…

Previous filtering for mtu (netdev_dpdk_set_mtu) and max queues (dpdk_eth_dev_init)/max buf desc (dpdk_process_queue_size)
enforcement (present and also any future design) should prevent snprintf from having an unexpected return value number of chars, which would
mean snprintf should only have an unexpected return value number of chars, if a future coding error were to be introduced.
FYI, OVS traditionally deals with other such cases with ovs_assert(), not a VLOG_DBG, since it is easier to find the bug earlier.
I believe snprintf at this level should not be catching user config errors.



    +        VLOG_DBG("snprintf returned %d. Failed to generate a mempool "
    +            "name for \"%s\". Hash:0x%x, mtu:%d, mbufs:%u.",
    +            ret, dmp->if_name, h, dmp->mtu, dmp->mp_size);
             return NULL;
         }
         return mp_name;
    @@ -533,6 +536,10 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu, bool *mp_exists)
     
         do {
             char *mp_name = dpdk_mp_name(dmp);
    +        if (!mp_name) {
    +            rte_free(dmp);
    +            return NULL;
    +        }
     
             VLOG_DBG("Port %s: Requesting a mempool of %u mbufs "
                       "on socket %d for %d Rx and %d Tx queues.",
    -- 
    2.4.11
    
    _______________________________________________
    dev mailing list
    dev at openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=cKHgut93ZGYQzoLXpXQWPqcZhBX3NMdQaRnbNlfvhiU&s=tx4JYNmCFZ-vgGQzTIteThFJN2RSlMtqg34oTwlFEF0&e=