Akihiko,
This series was already discussed several months ago.
I'd suggest to postpone commenting on it and resume them after merging.
Thanks for understanding.
Yuri
On Mon, Dec 11, 2023 at 3:05 PM Akihiko Odaki <akihiko.od...@gmail.com>
wrote:
> On 2023/08/31 15:51, Andrew Melnychenko wrote:
> > Changed eBPF map updates through mmaped array.
> > Mmaped arrays provide direct access to map data.
> > It should omit using bpf_map_update_elem() call,
> > which may require capabilities that are not present.
> >
> > Signed-off-by: Andrew Melnychenko <and...@daynix.com>
> > ---
> > ebpf/ebpf_rss.c | 117 ++++++++++++++++++++++++++++++++++++++----------
> > ebpf/ebpf_rss.h | 5 +++
> > 2 files changed, 99 insertions(+), 23 deletions(-)
> >
> > diff --git a/ebpf/ebpf_rss.c b/ebpf/ebpf_rss.c
> > index cee658c158..247f5eee1b 100644
> > --- a/ebpf/ebpf_rss.c
> > +++ b/ebpf/ebpf_rss.c
> > @@ -27,19 +27,83 @@ void ebpf_rss_init(struct EBPFRSSContext *ctx)
> > {
> > if (ctx != NULL) {
> > ctx->obj = NULL;
> > + ctx->program_fd = -1;
> > + ctx->map_configuration = -1;
> > + ctx->map_toeplitz_key = -1;
> > + ctx->map_indirections_table = -1;
> > +
> > + ctx->mmap_configuration = NULL;
> > + ctx->mmap_toeplitz_key = NULL;
> > + ctx->mmap_indirections_table = NULL;
> > }
> > }
> >
> > bool ebpf_rss_is_loaded(struct EBPFRSSContext *ctx)
> > {
> > - return ctx != NULL && ctx->obj != NULL;
> > + return ctx != NULL && (ctx->obj != NULL || ctx->program_fd != -1);
> > +}
> > +
> > +static bool ebpf_rss_mmap(struct EBPFRSSContext *ctx)
> > +{
> > + if (!ebpf_rss_is_loaded(ctx)) {
> > + return false;
> > + }
> > +
> > + ctx->mmap_configuration = mmap(NULL, qemu_real_host_page_size(),
> > + PROT_READ | PROT_WRITE, MAP_SHARED,
> > + ctx->map_configuration, 0);
> > + if (ctx->mmap_configuration == MAP_FAILED) {
> > + trace_ebpf_error("eBPF RSS", "can not mmap eBPF configuration
> array");
> > + return false;
> > + }
> > + ctx->mmap_toeplitz_key = mmap(NULL, qemu_real_host_page_size(),
> > + PROT_READ | PROT_WRITE, MAP_SHARED,
> > + ctx->map_toeplitz_key, 0);
> > + if (ctx->mmap_toeplitz_key == MAP_FAILED) {
> > + trace_ebpf_error("eBPF RSS", "can not mmap eBPF toeplitz key");
> > + goto toeplitz_fail;
> > + }
> > + ctx->mmap_indirections_table = mmap(NULL,
> qemu_real_host_page_size(),
> > + PROT_READ | PROT_WRITE, MAP_SHARED,
> > + ctx->map_indirections_table, 0);
> > + if (ctx->mmap_indirections_table == MAP_FAILED) {
> > + trace_ebpf_error("eBPF RSS", "can not mmap eBPF indirection
> table");
> > + goto indirection_fail;
> > + }
> > +
> > + return true;
> > +
> > +indirection_fail:
> > + munmap(ctx->mmap_toeplitz_key, qemu_real_host_page_size());
> > +toeplitz_fail:
> > + munmap(ctx->mmap_configuration, qemu_real_host_page_size());
> > +
> > + ctx->mmap_configuration = NULL;
> > + ctx->mmap_toeplitz_key = NULL;
> > + ctx->mmap_indirections_table = NULL;
>
> What about:
>
> > +indirection_fail:
> > + munmap(ctx->mmap_toeplitz_key, qemu_real_host_page_size());
> > + ctx->mmap_toeplitz_key = NULL;
> > +toeplitz_fail:
> > + munmap(ctx->mmap_configuration, qemu_real_host_page_size());
> > + ctx->mmap_configuration = NULL;
>
> It will be clearer when the pointer becomes invalid this way.
>
> > + return false;
> > +}
> > +
> > +static void ebpf_rss_munmap(struct EBPFRSSContext *ctx)
> > +{
> > + if (!ebpf_rss_is_loaded(ctx)) {
> > + return;
> > + }
> > +
> > + munmap(ctx->mmap_indirections_table, qemu_real_host_page_size());
> > + munmap(ctx->mmap_toeplitz_key, qemu_real_host_page_size());
> > + munmap(ctx->mmap_configuration, qemu_real_host_page_size());
> > +
> > + ctx->mmap_configuration = NULL;
> > + ctx->mmap_toeplitz_key = NULL;
> > + ctx->mmap_indirections_table = NULL;
> > }
> >
> > bool ebpf_rss_load(struct EBPFRSSContext *ctx)
> > {
> > struct rss_bpf *rss_bpf_ctx;
> >
> > - if (ctx == NULL) {
> > + if (ctx == NULL || ebpf_rss_is_loaded(ctx)) {
> > return false;
> > }
>
> You can omit ctx == NULL just as you do for ebpf_rss_munmap().
>
> >
> > @@ -66,10 +130,18 @@ bool ebpf_rss_load(struct EBPFRSSContext *ctx)
> > ctx->map_toeplitz_key = bpf_map__fd(
> > rss_bpf_ctx->maps.tap_rss_map_toeplitz_key);
> >
> > + if (!ebpf_rss_mmap(ctx)) {
> > + goto error;
> > + }
> > +
> > return true;
> > error:
> > rss_bpf__destroy(rss_bpf_ctx);
> > ctx->obj = NULL;
> > + ctx->program_fd = -1;
> > + ctx->map_configuration = -1;
> > + ctx->map_toeplitz_key = -1;
> > + ctx->map_indirections_table = -1;
> >
> > return false;
> > }
> > @@ -77,15 +149,11 @@ error:
> > static bool ebpf_rss_set_config(struct EBPFRSSContext *ctx,
> > struct EBPFRSSConfig *config)
> > {
> > - uint32_t map_key = 0;
> > -
> > if (!ebpf_rss_is_loaded(ctx)) {
> > return false;
> > }
> > - if (bpf_map_update_elem(ctx->map_configuration,
> > - &map_key, config, 0) < 0) {
> > - return false;
> > - }
> > +
> > + memcpy(ctx->mmap_configuration, config, sizeof(*config));
> > return true;
> > }
> >
> > @@ -93,27 +161,19 @@ static bool ebpf_rss_set_indirections_table(struct
> EBPFRSSContext *ctx,
> > uint16_t
> *indirections_table,
> > size_t len)
> > {
> > - uint32_t i = 0;
> > -
> > if (!ebpf_rss_is_loaded(ctx) || indirections_table == NULL ||
> > len > VIRTIO_NET_RSS_MAX_TABLE_LEN) {
> > return false;
> > }
> >
> > - for (; i < len; ++i) {
> > - if (bpf_map_update_elem(ctx->map_indirections_table, &i,
> > - indirections_table + i, 0) < 0) {
> > - return false;
> > - }
> > - }
> > + memcpy(ctx->mmap_indirections_table, indirections_table,
> > + sizeof(*indirections_table) * len);
>
> This line is not aligned with the parenthesis on the previous line.
>
