From: "Rong, Leyi" <leyi.rong@intel.com>
To: "Richardson, Bruce" <bruce.richardson@intel.com>
Cc: "ferruh.yigit@xilinx.com" <ferruh.yigit@xilinx.com>,
"david.marchand@redhat.com" <david.marchand@redhat.com>,
"suanmingm@nvidia.com" <suanmingm@nvidia.com>,
"Wang, Yipeng1" <yipeng1.wang@intel.com>,
"zaoxingliu@gmail.com" <zaoxingliu@gmail.com>,
"Gobriel, Sameh" <sameh.gobriel@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>
Subject: RE: [PATCH v4 1/2] member: implement NitroSketch mode
Date: Fri, 16 Sep 2022 02:30:27 +0000 [thread overview]
Message-ID: <MW4PR11MB5774A9317157EB8905C2901DEC489@MW4PR11MB5774.namprd11.prod.outlook.com> (raw)
In-Reply-To: <YyMy5O1tcZcn/C8z@bricha3-MOBL.ger.corp.intel.com>
Hi Bruce,
The comments will be addressed in v5, thank you!
> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Thursday, September 15, 2022 10:13 PM
> To: Rong, Leyi <leyi.rong@intel.com>
> Cc: ferruh.yigit@xilinx.com; david.marchand@redhat.com;
> suanmingm@nvidia.com; Wang, Yipeng1 <yipeng1.wang@intel.com>;
> zaoxingliu@gmail.com; Gobriel, Sameh <sameh.gobriel@intel.com>;
> dev@dpdk.org
> Subject: Re: [PATCH v4 1/2] member: implement NitroSketch mode
>
> On Thu, Sep 15, 2022 at 09:03:41PM +0800, Leyi Rong wrote:
> > Sketching algorithm provide high-fidelity approximate measurements and
> > appears as a promising alternative to traditional approaches such as
> > packet sampling.
> >
> > NitroSketch [1] is a software sketching framework that optimizes
> > performance, provides accuracy guarantees, and supports a variety of
> > sketches.
> >
> > This commit adds a new data structure called sketch into membership
> > library. This new data structure is an efficient way to profile the
> > traffic for heavy hitters. Also use min-heap structure to maintain the
> > top-k flow keys.
> >
> > [1] Zaoxing Liu, Ran Ben-Basat, Gil Einziger, Yaron Kassner, Vladimir
> > Braverman, Roy Friedman, Vyas Sekar, "NitroSketch: Robust and General
> > Sketch-based Monitoring in Software Switches", in ACM SIGCOMM 2019.
> > https://dl.acm.org/doi/pdf/10.1145/3341302.3342076
> >
> > Signed-off-by: Alan Liu <zaoxingliu@gmail.com>
> > Signed-off-by: Yipeng Wang <yipeng1.wang@intel.com>
> > Signed-off-by: Leyi Rong <leyi.rong@intel.com>
> > ---
>
> Couple of comments below on the meson.build file.
>
> /Bruce
>
> > lib/member/meson.build | 42 +-
> > lib/member/rte_member.c | 75 ++++
> > lib/member/rte_member.h | 154 ++++++-
> > lib/member/rte_member_heap.h | 424 ++++++++++++++++++
> > lib/member/rte_member_sketch.c | 594 ++++++++++++++++++++++++++
> > lib/member/rte_member_sketch.h | 97 +++++
> > lib/member/rte_member_sketch_avx512.c | 70 +++
> > lib/member/rte_member_sketch_avx512.h | 35 ++
> > lib/member/rte_xxh64_avx512.h | 117 +++++
> > lib/member/version.map | 9 +
> > 10 files changed, 1613 insertions(+), 4 deletions(-) create mode
> > 100644 lib/member/rte_member_heap.h create mode 100644
> > lib/member/rte_member_sketch.c create mode 100644
> > lib/member/rte_member_sketch.h create mode 100644
> > lib/member/rte_member_sketch_avx512.c
> > create mode 100644 lib/member/rte_member_sketch_avx512.h
> > create mode 100644 lib/member/rte_xxh64_avx512.h
> >
> > diff --git a/lib/member/meson.build b/lib/member/meson.build index
> > e06fddc240..3df0fbc7a6 100644
> > --- a/lib/member/meson.build
> > +++ b/lib/member/meson.build
> > @@ -7,6 +7,46 @@ if is_windows
> > subdir_done()
> > endif
> >
> > -sources = files('rte_member.c', 'rte_member_ht.c',
> > 'rte_member_vbf.c')
> > +sources = files('rte_member.c', 'rte_member_ht.c',
> > +'rte_member_vbf.c', 'rte_member_sketch.c')
>
> At this point, with 4 entries I'd suggest reworking this to a vertical list.
>
> sources = files(
> 'rte_member.c',
> ....
> )
>
> > headers = files('rte_member.h')
> > deps += ['hash']
> > +includes += include_directories('../hash', '../ring')
> > +
>
> Use dependencies rather than include-paths. Having a dependency on hash
> means that your code will be compiled with the hash include paths already
> present. Similarly you can add "ring" to the deps variable and drop this line.
>
> > +# compile AVX512 version if:
> > +# we are building 64-bit binary AND binutils can generate proper code
>
> I think this comment is no longer necessary. The condition itself is clear.
> The following comment can be kept though as it explains what is being done at a
> higher level than the code.
>
> > +if dpdk_conf.has('RTE_ARCH_X86_64') and binutils_ok
> > + # compile AVX512 version if either:
> > + # a. we have AVX512 supported in minimum instruction set
> > + # baseline
> > + # b. it's not minimum instruction set, but supported by
> > + # compiler
> > + #
> > + # in former case, just add avx512 C file to files list
> > + # in latter case, compile c file to static lib, using correct
> > + # compiler flags, and then have the .o file from static lib
> > + # linked into main lib.
> > +
> > + #check if all required flags already enabled
> > + sketch_avx512_flags = ['__AVX512F__', '__AVX512DQ__',
> > + '__AVX512IFMA__']
> > +
> > + sketch_avx512_on = true
> > + foreach f:sketch_avx512_flags
> > + if cc.get_define(f, args: machine_args) == ''
> > + sketch_avx512_on = false
> > + endif
> > + endforeach
> > +
> > + if sketch_avx512_on == true
> > + cflags += ['-DCC_AVX512_SUPPORT']
> > + sources += files('rte_member_sketch_avx512.c')
> > + elif cc.has_multi_arguments('-mavx512f', '-mavx512dq', '-mavx512ifma')
> > + sketch_avx512_tmp = static_library('sketch_avx512_tmp',
> > + 'rte_member_sketch_avx512.c',
> > + include_directories: includes,
> > + dependencies: static_rte_eal,
> > + c_args: cflags +
> > + ['-mavx512f', '-mavx512dq', '-mavx512ifma'])
> > + objs +=
> sketch_avx512_tmp.extract_objects('rte_member_sketch_avx512.c')
> > + cflags += ['-DCC_AVX512_SUPPORT']
>
> Logic looks correct here now, but beware of your indentation. You are mixing
> tabs and spaces. Meson files should have spaces only, no tabs.
>
> > + endif
> > +endif
next prev parent reply other threads:[~2022-09-16 2:30 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-10 7:45 [PATCH 0/2] introduce NitroSketch Mode into membership library Leyi Rong
2022-08-10 7:45 ` [PATCH 1/2] member: implement NitroSketch mode Leyi Rong
2022-08-16 8:59 ` Ferruh Yigit
2022-08-26 2:32 ` Rong, Leyi
2022-08-10 7:45 ` [PATCH 2/2] test/member: add functional and perf tests for sketch Leyi Rong
2022-08-11 2:33 ` Suanming Mou
2022-08-26 2:26 ` Rong, Leyi
2022-08-31 6:46 ` [PATCH v2 0/2] introduce NitroSketch Mode into membership library Leyi Rong
2022-08-31 6:46 ` [PATCH v2 1/2] member: implement NitroSketch mode Leyi Rong
2022-09-13 14:56 ` David Marchand
2022-09-14 9:42 ` Rong, Leyi
2022-08-31 6:46 ` [PATCH v2 2/2] test/member: add functional and perf tests for sketch Leyi Rong
2022-09-15 2:14 ` [PATCH v3 0/2] introduce NitroSketch Mode into membership library Leyi Rong
2022-09-15 2:14 ` [PATCH v3 1/2] member: implement NitroSketch mode Leyi Rong
2022-09-15 7:47 ` David Marchand
2022-09-15 13:02 ` Rong, Leyi
2022-09-15 2:14 ` [PATCH v3 2/2] test/member: add functional and perf tests for sketch Leyi Rong
2022-09-15 7:30 ` Jiang, YuX
2022-09-15 7:41 ` David Marchand
2022-09-15 13:03 ` [PATCH v4 0/2] introduce NitroSketch Mode into membership library Leyi Rong
2022-09-15 13:03 ` [PATCH v4 1/2] member: implement NitroSketch mode Leyi Rong
2022-09-15 14:12 ` Bruce Richardson
2022-09-16 2:30 ` Rong, Leyi [this message]
2022-09-15 13:03 ` [PATCH v4 2/2] test/member: add functional and perf tests for sketch Leyi Rong
2022-09-16 3:03 ` [PATCH v5 0/2] introduce NitroSketch Mode into membership library Leyi Rong
2022-09-16 3:03 ` [PATCH v5 1/2] member: implement NitroSketch mode Leyi Rong
2022-09-22 3:00 ` Jiang, YuX
2022-10-03 12:37 ` Thomas Monjalon
2022-10-08 3:36 ` Rong, Leyi
2022-09-16 3:03 ` [PATCH v5 2/2] test/member: add functional and perf tests for sketch Leyi Rong
2022-10-09 23:13 ` [PATCH v5 0/2] introduce NitroSketch Mode into membership library Thomas Monjalon
2022-10-10 2:36 ` Rong, Leyi
2022-10-10 7:30 ` Thomas Monjalon
2022-10-10 8:28 ` Rong, Leyi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MW4PR11MB5774A9317157EB8905C2901DEC489@MW4PR11MB5774.namprd11.prod.outlook.com \
--to=leyi.rong@intel.com \
--cc=bruce.richardson@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@xilinx.com \
--cc=sameh.gobriel@intel.com \
--cc=suanmingm@nvidia.com \
--cc=yipeng1.wang@intel.com \
--cc=zaoxingliu@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).