Re: [dpdk-stable] [dpdk-dev] [PATCH] nfp: handle packets with length 0 as usual ones

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 8/21/2017 2:08 PM, Alejandro Lucero wrote:
> 
> 
> On Mon, Aug 21, 2017 at 11:34 AM, Ferruh Yigit <ferruh.yigit@intel.com
> <mailto:ferruh.yigit@intel.com>> wrote:
> 
>     On 8/18/2017 5:23 PM, Alejandro Lucero wrote:
>     >
>     >
>     > On Fri, Aug 18, 2017 at 4:10 PM, Ferruh Yigit <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>
>     > <mailto:ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>>> wrote:
>     >
>     >     On 8/11/2017 11:05 AM, Alejandro Lucero wrote:
>     >     > A DPDK app could, whatever the reason, send packets with size 0.
>     >     > The PMD is not sending those packets, which does make sense,
>     >     > but the problem is the mbuf is not released either. That leads
>     >     > to mbufs not being available, because the app trusts the
>     >     > PMD will do it.
>     >     >
>     >     > Although this is a problem related to app wrong behaviour, we
>     >     > should harden the PMD in this regard. Not sending a packet with
>     >     > size 0 could be problematic, needing special handling inside the
>     >     > PMD xmit function. It could be a burst of those packets, which can
>     >     > be easily handled, but it could also be a single packet in a burst,
>     >     > what is harder to handle.
>     >     >
>     >     > It would be simpler to just send that kind of packets, which will
>     >     > likely be dropped by the hw at some point. The main problem is how
>     >     > the fw/hw handles the DMA, because a dma read to a hypothetical 0x0
>     >     > address could trigger an IOMMU error. It turns out, it is safe to
>     >     > send a descriptor with packet size 0 to the hardware: the DMA never
>     >     > happens, from the PCIe point of view.
>     >     >
>     >     > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com <mailto:alejandro.lucero@netronome.com>
>     >     <mailto:alejandro.lucero@netronome.com
>     <mailto:alejandro.lucero@netronome.com>>>
>     >     > ---
>     >     >  drivers/net/nfp/nfp_net.c | 17 ++++++++++++-----
>     >     >  1 file changed, 12 insertions(+), 5 deletions(-)
>     >     >
>     >     > diff --git a/drivers/net/nfp/nfp_net.c
>     b/drivers/net/nfp/nfp_net.c
>     >     > index 92b03c4..679a91b 100644
>     >     > --- a/drivers/net/nfp/nfp_net.c
>     >     > +++ b/drivers/net/nfp/nfp_net.c
>     >     > @@ -2094,7 +2094,7 @@ uint32_t nfp_net_txq_full(struct
>     nfp_net_txq
>     >     *txq)
>     >     >                */
>     >     >               pkt_size = pkt->pkt_len;
>     >     >
>     >     > -             while (pkt_size) {
>     >     > +             while (pkt) {
>     >     >                       /* Copying TSO, VLAN and cksum info */
>     >     >                       *txds = txd;
>     >     >
>     >     > @@ -2126,17 +2126,24 @@ uint32_t nfp_net_txq_full(struct
>     >     nfp_net_txq *txq)
>     >     >                               txq->wr_p = 0;
>     >     >
>     >     >                       pkt_size -= dma_size;
>     >     > -                     if (!pkt_size) {
>     >     > +                     if (!pkt_size)
>     >     >                               /* End of packet */
>     >     >                               txds->offset_eop |=
>     PCIE_DESC_TX_EOP;
>     >     > -                     } else {
>     >     > +                     else
>     >     >                               txds->offset_eop &=
>     >     PCIE_DESC_TX_OFFSET_MASK;
>     >     > -                             pkt = pkt->next;
>     >     > -                     }
>     >     > +
>     >     > +                     pkt = pkt->next;
>     >     >                       /* Referencing next free TX descriptor */
>     >     >                       txds = &txq->txds[txq->wr_p];
>     >     >                       lmbuf = &txq->txbufs[txq->wr_p].mbuf;
>     >     >                       issued_descs++;
>     >     > +
>     >     > +                     /* Double-checking if we have to use
>     chained
>     >     mbuf.
>     >     > +                      * It seems there are some apps which
>     could
>     >     wrongly
>     >     > +                      * have zeroed mbufs chained leading
>     to send
>     >     null
>     >     > +                      * descriptors to the hw. */
>     >     > +                     if (!pkt_size)
>     >     > +                             break;
>     >
>     >     For the case chained mbufs with all are zero size [1], won't
>     this cause
>     >     next mbufs not freed because rte_pktmbuf_free_seg(*lmbuf) used?
>     >
>     >
>     > Good point. Being honest, we had the problem with mbufs and size
>     0, and
>     > this last check
>     > was not initially there. But we saw performance being low after the
>     > change, and the only thing
>     > which could explain it was this sort of chained mbufs. There was not
>     > mbuf allocation problem at
>     > all. It was like more (null) packets being sent to the hardware now.
>     > This last check solved the
>     > performance problem.
> 
>     I assume performance problem is with the chained mbufs with 0 size, I
>     believe this should be fixed in application, not in PMD level.
> 
>     And if application is sending chained mbufs with 0 size, with above code
>     it will eventually be out off mbufs, since they are not freed, and same
>     problem will occur that this patch is trying to avoid, but perhaps in
>     longer run.
> 
> 
> This is definitely an app problem and maybe that last check should be
> avoided and to process that chained mbuf, whatever is it coming from, if
> "pkt = pkt->next" is not null.
> 
> Are you OK of I send another version without that last if clause?

Yes, thank you.

>  
>  
> 
>     >
>     > Once I have said that, I have to admit my explanation implies some
>     > serious problem when
>     > handling mbufs, and something the app is doing really badly, so I
>     could
>     > understand someone
>     > saying this is hidden a serious problem and should not be there. 
>     >
>     >     [1]
>     >     As you mentioned in the commit log, this not correct thing to
>     do, but
>     >     since patch is trying to harden PMD for this wrong application
>     >     behavior..
>     >
>     >
>     > If you consider this last check should not be there, I'll be glad to
>     > remove it.
>     >  
>     >
>     >
>     >     >               }
>     >     >               i++;
>     >     >       }
>     >     >
>     >
>     >
> 
>