Re: [dpdk-dev] [PATCH v5 08/17] fm10k: add RX/TX single queue start/stop function

[Jeff Shaw <jeffrey.b.shaw@intel.com>]

Hi David, thanks for the review.

On Fri, Feb 13, 2015 at 12:31:16PM +0100, David Marchand wrote:
> Hello,
> 
> On Fri, Feb 13, 2015 at 9:19 AM, Chen Jing D(Mark) <jing.d.chen@intel.com>
> wrote:
> 
> [snip]
> 
> +/*
> > + * Verify Rx packet buffer alignment is valid.
> > + *
> > + * Hardware requires specific alignment for Rx packet buffers. At
> > + * least one of the following two conditions must be satisfied.
> > + *  1. Address is 512B aligned
> > + *  2. Address is 8B aligned and buffer does not cross 4K boundary.
> > + *
> > + * Return 1 if buffer alignment satisfies at least one condition,
> > + * otherwise return 0.
> > + *
> > + * Note: Alignment is checked by the driver when the Rx queue is reset. It
> > + *       is assumed that if an entire descriptor ring can be filled with
> > + *       buffers containing valid alignment, then all buffers in that
> > mempool
> > + *       have valid address alignment. It is the responsibility of the
> > user
> > + *       to ensure all buffers have valid alignment, as it is the user who
> > + *       creates the mempool.
> > + * Note: It is assumed the buffer needs only to store a maximum size
> > Ethernet
> > + *       frame.
> > + */
> > +static inline int
> > +fm10k_addr_alignment_valid(struct rte_mbuf *mb)
> > +{
> > +       uint64_t addr = MBUF_DMA_ADDR_DEFAULT(mb);
> > +       uint64_t boundary1, boundary2;
> > +
> > +       /* 512B aligned? */
> > +       if (RTE_ALIGN(addr, 512) == addr)
> > +               return 1;
> > +
> > +       /* 8B aligned, and max Ethernet frame would not cross a 4KB
> > boundary? */
> > +       if (RTE_ALIGN(addr, 8) == addr) {
> > +               boundary1 = RTE_ALIGN_FLOOR(addr, 4096);
> > +               boundary2 = RTE_ALIGN_FLOOR(addr +
> > ETHER_MAX_VLAN_FRAME_LEN,
> > +                                               4096);
> > +               if (boundary1 == boundary2)
> > +                       return 1;
> > +       }
> > +
> > +       /* use RTE_LOG directly to make sure this error is seen */
> > +       RTE_LOG(ERR, PMD, "%s(): Error: Invalid buffer alignment\n",
> > __func__);
> > +
> > +       return 0;
> > +}
> >
> 
> Same comment as before, do not directly use RTE_LOG.
> This is init stuff, you have a PMD_INIT_LOG macro.
Agreed, the comment should be fixed.

> 
> By the way, I need to dig deeper into this, but I can see multiple patches
> ensuring buffer alignment.
> Do we really need to validate this alignment here, if we already validated
> this constraint at the mempool level ?
> 

This is really a sanity check. The buffer alignment needs to be
checked at runtime becuase a user could modify the alignment. We
provide a check here to be extra safe, and hopefully to fail at
init time rather than later.

There are two ways to satisfy the alignment requirements for the
hardware. Currently the driver implements the 512B alignment, but
it is possible somebody may want to the other 8B alignment w/o
crossing a 4K page boundary.  This sanity check would help catch
any possible issues in the future related to buffer alignment.

-Jeff