On Mon, Mar 07, 2016 at 02:32:46AM +0000, Xie, Huawei wrote:
> On 3/4/2016 10:15 AM, Yuanhan Liu wrote:
> > On Thu, Mar 03, 2016 at 04:30:42PM +0000, Xie, Huawei wrote:
> >> On 2/18/2016 9:48 PM, Yuanhan Liu wrote:
> >>> + mbuf_avail = 0;
> >>> + mbuf_offset = 0;
> >>> + while (desc_avail || (desc->flags & VRING_DESC_F_NEXT) != 0) {
> >>> + /* This desc reachs to its end, get the next one */
> >>> + if (desc_avail == 0) {
> >>> + desc = &vq->desc[desc->next];
> >>> +
> >>> + desc_addr = gpa_to_vva(dev, desc->addr);
> >>> + rte_prefetch0((void *)(uintptr_t)desc_addr);
> >>> +
> >>> + desc_offset = 0;
> >>> + desc_avail = desc->len;
> >>> +
> >>> + PRINT_PACKET(dev, (uintptr_t)desc_addr, desc->len, 0);
> >>> + }
> >>> +
> >>> + /*
> >>> + * This mbuf reachs to its end, get a new one
> >>> + * to hold more data.
> >>> + */
> >>> + if (mbuf_avail == 0) {
> >>> + cur = rte_pktmbuf_alloc(mbuf_pool);
> >>> + if (unlikely(!cur)) {
> >>> + RTE_LOG(ERR, VHOST_DATA, "Failed to "
> >>> + "allocate memory for mbuf.\n");
> >>> + if (head)
> >>> + rte_pktmbuf_free(head);
> >>> + return NULL;
> >>> + }
> >> We could always allocate the head mbuf before the loop, then we save the
> >> following branch and make the code more streamlined.
> >> It reminds me that this change prevents the possibility of mbuf bulk
> >> allocation, one solution is we pass the head mbuf from an additional
> >> parameter.
> > Yep, that's also something I have thought of.
> >
> >> Btw, put unlikely before the check of mbuf_avail and checks elsewhere.
> > I don't think so. It would benifit for the small packets. What if,
> > however, when TSO or jumbo frame is enabled that we have big packets?
>
> Prefer to favor the path that packet could fit in one mbuf.
Hmmm, why? While I know that TSO and mergeable buf is disable by default
in our vhost example vhost-switch, they are enabled by default in real
life.
> Btw, not specially to this, return "m = copy_desc_to_mbuf(dev, vq,
> desc_indexes[i], mbuf_pool)", failure case is unlikely to happen, so add
> unlikely for the check if (m == NULL) there. Please check all branches
> elsewhere.
Thanks for the remind, will have a detail check.
--yliu
