Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210128001948.1637901-1-songliubraving@fb.com> <20210128001948.1637901-2-songliubraving@fb.com>
In-Reply-To: <20210128001948.1637901-2-songliubraving@fb.com>
From:   KP Singh <kpsingh@kernel.org>
Date:   Mon, 1 Feb 2021 02:20:46 +0100
Message-ID: <CACYkzJ4Ha1r4oqPge7yJjORdBUPg=huHSjER58ka24OEw_4S0A@mail.gmail.com>
Subject: Re: [PATCH v3 bpf-next 1/4] bpf: enable task local storage for
 tracing programs
To:     Song Liu <songliubraving@fb.com>
Cc:     bpf <bpf@vger.kernel.org>, Networking <netdev@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>, mingo@redhat.com,
        Peter Zijlstra <peterz@infradead.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Kernel Team <kernel-team@fb.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jan 28, 2021 at 1:20 AM Song Liu <songliubraving@fb.com> wrote:
>
> To access per-task data, BPF programs usually creates a hash table with
> pid as the key. This is not ideal because:
>  1. The user need to estimate the proper size of the hash table, which may
>     be inaccurate;
>  2. Big hash tables are slow;
>  3. To clean up the data properly during task terminations, the user need
>     to write extra logic.
>
> Task local storage overcomes these issues and offers a better option for
> these per-task data. Task local storage is only available to BPF_LSM. Now
> enable it for tracing programs.
>
> Unlike LSM progreams, tracing programs can be called in IRQ contexts.

nit: typo *programs

> Helpers that accesses task local storage are updated to use

nit: Helpers that access..

> raw_spin_lock_irqsave() instead of raw_spin_lock_bh().
>
> Tracing programs can attach to functions on the task free path, e.g.
> exit_creds(). To avoid allocating task local storage after
> bpf_task_storage_free(). bpf_task_storage_get() is updated to not allocate
> new storage when the task is not refcounted (task->usage == 0).
>
> Signed-off-by: Song Liu <songliubraving@fb.com>

Acked-by: KP Singh <kpsingh@kernel.org>

Thanks for adding better commit descriptions :)

I think checking the usage before adding storage should work for the
task exit path (I could not think of cases where it would break).
Would also be nice to check with Martin and Hao about this.