Re: [PATCH] replaced O(n^2) sort in sort_by_physaddr() with qsort() from standard library

[Jay Rolette <rolette-bIuJOMs36aleGPcbtGPokg@public.gmane.org>]

Hi Thomas,

Please read http://dpdk.org/dev#send for submission guidelines.
>

I did when I was figuring out how to submit the patch, but possible I'm
missing something on the tools to get it to include the commit comment
correctly.

A description of why you do it would be welcome in the commit log.
>

How much are you looking for here? I thought replacing an O(n^2) algorithm
with qsort() was fairly self-evident. Less code and take advantage of
standard library code that is faster.

I get it in the general case. Just didn't seem necessary for this one.


> > +static int
> > +cmp_physaddr(const void *a, const void *b)
> > +{
> > +#ifndef RTE_ARCH_PPC_64
> > +     const struct hugepage_file *p1 = (const struct hugepage_file *)a;
> > +     const struct hugepage_file *p2 = (const struct hugepage_file *)b;
> > +#else
> > +     // PowerPC needs memory sorted in reverse order from x86
>
> Comments shall be C-style (/* */).
>

Single line comments ('//') have been part of the C standard since C99. Is
DPDK following C89 or is this just a style thing? If it is a style thing, a
link to a page with the rubric would be helpful. I didn't see one on the
submission guidelines.


> > +     const struct hugepage_file *p1 = (const struct hugepage_file *)b;
> > +     const struct hugepage_file *p2 = (const struct hugepage_file *)a;
> > +#endif
> > +     if (p1->physaddr < p2->physaddr)
> > +             return -1;
> > +     else if (p1->physaddr > p2->physaddr)
> > +             return 1;
> > +     else
> > +             return 0;
> > +}
>
> One of the goal of EAL is to avoid #ifdef.
> So that function would probably be better located in
> lib/librte_eal/common/include/arch/* with different implemenations
> depending of the architecture.
>

Hmm... I was following the approach already used in the module. See
map_all_hugepages(), remap_all_hugepages().

Regards,
Jay