| Message ID | 20200727184506.2279656-2-guro@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: switch to memcg-based memory accounting | expand |
On Mon, Jul 27, 2020 at 12:20 PM Roman Gushchin <guro@fb.com> wrote: > > Include memory used by bpf programs into the memcg-based accounting. > This includes the memory used by programs itself, auxiliary data > and statistics. > > Signed-off-by: Roman Gushchin <guro@fb.com> > --- > kernel/bpf/core.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c > index bde93344164d..daab8dcafbd4 100644 > --- a/kernel/bpf/core.c > +++ b/kernel/bpf/core.c > @@ -77,7 +77,7 @@ void *bpf_internal_load_pointer_neg_helper(const struct sk_buff *skb, int k, uns > > struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flags) > { > - gfp_t gfp_flags = GFP_KERNEL | __GFP_ZERO | gfp_extra_flags; > + gfp_t gfp_flags = GFP_KERNEL_ACCOUNT | __GFP_ZERO | gfp_extra_flags; > struct bpf_prog_aux *aux; > struct bpf_prog *fp; > > @@ -86,7 +86,7 @@ struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flag > if (fp == NULL) > return NULL; > > - aux = kzalloc(sizeof(*aux), GFP_KERNEL | gfp_extra_flags); > + aux = kzalloc(sizeof(*aux), GFP_KERNEL_ACCOUNT | gfp_extra_flags); > if (aux == NULL) { > vfree(fp); > return NULL; > @@ -104,7 +104,7 @@ struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flag > > struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags) > { > - gfp_t gfp_flags = GFP_KERNEL | __GFP_ZERO | gfp_extra_flags; > + gfp_t gfp_flags = GFP_KERNEL_ACCOUNT | __GFP_ZERO | gfp_extra_flags; > struct bpf_prog *prog; > int cpu; > > @@ -217,7 +217,7 @@ void bpf_prog_free_linfo(struct bpf_prog *prog) > struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size, > gfp_t gfp_extra_flags) > { > - gfp_t gfp_flags = GFP_KERNEL | __GFP_ZERO | gfp_extra_flags; > + gfp_t gfp_flags = GFP_KERNEL_ACCOUNT | __GFP_ZERO | gfp_extra_flags; > struct bpf_prog *fp; > u32 pages, delta; > int ret; > -- Do we need similar changes in bpf_prog_array_copy() bpf_prog_alloc_jited_linfo() bpf_prog_clone_create() and maybe a few more? Thanks, Song
On Mon, Jul 27, 2020 at 03:11:42PM -0700, Song Liu wrote: > On Mon, Jul 27, 2020 at 12:20 PM Roman Gushchin <guro@fb.com> wrote: > > > > Include memory used by bpf programs into the memcg-based accounting. > > This includes the memory used by programs itself, auxiliary data > > and statistics. > > > > Signed-off-by: Roman Gushchin <guro@fb.com> > > --- > > kernel/bpf/core.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c > > index bde93344164d..daab8dcafbd4 100644 > > --- a/kernel/bpf/core.c > > +++ b/kernel/bpf/core.c > > @@ -77,7 +77,7 @@ void *bpf_internal_load_pointer_neg_helper(const struct sk_buff *skb, int k, uns > > > > struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flags) > > { > > - gfp_t gfp_flags = GFP_KERNEL | __GFP_ZERO | gfp_extra_flags; > > + gfp_t gfp_flags = GFP_KERNEL_ACCOUNT | __GFP_ZERO | gfp_extra_flags; > > struct bpf_prog_aux *aux; > > struct bpf_prog *fp; > > > > @@ -86,7 +86,7 @@ struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flag > > if (fp == NULL) > > return NULL; > > > > - aux = kzalloc(sizeof(*aux), GFP_KERNEL | gfp_extra_flags); > > + aux = kzalloc(sizeof(*aux), GFP_KERNEL_ACCOUNT | gfp_extra_flags); > > if (aux == NULL) { > > vfree(fp); > > return NULL; > > @@ -104,7 +104,7 @@ struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flag > > > > struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags) > > { > > - gfp_t gfp_flags = GFP_KERNEL | __GFP_ZERO | gfp_extra_flags; > > + gfp_t gfp_flags = GFP_KERNEL_ACCOUNT | __GFP_ZERO | gfp_extra_flags; > > struct bpf_prog *prog; > > int cpu; > > > > @@ -217,7 +217,7 @@ void bpf_prog_free_linfo(struct bpf_prog *prog) > > struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size, > > gfp_t gfp_extra_flags) > > { > > - gfp_t gfp_flags = GFP_KERNEL | __GFP_ZERO | gfp_extra_flags; > > + gfp_t gfp_flags = GFP_KERNEL_ACCOUNT | __GFP_ZERO | gfp_extra_flags; > > struct bpf_prog *fp; > > u32 pages, delta; > > int ret; > > -- Hi Song! Thank you for looking into the patchset! > > Do we need similar changes in > > bpf_prog_array_copy() > bpf_prog_alloc_jited_linfo() > bpf_prog_clone_create() > > and maybe a few more? I've tried to follow the rlimit-based accounting, so those objects which were skipped are mostly skipped now and vice versa. The main reason for that is simple: I don't know many parts of bpf code well enough to decide whether we need accounting or not. In general with memcg-based accounting we can easily cover places which were not covered previously: e.g. the memory used by the verifier. But I guess it's better to do it case-by-case. But if you're aware of any big objects which should be accounted for sure, please, let me know. Thanks!
On Mon, Jul 27, 2020 at 5:08 PM Roman Gushchin <guro@fb.com> wrote: > > On Mon, Jul 27, 2020 at 03:11:42PM -0700, Song Liu wrote: > > On Mon, Jul 27, 2020 at 12:20 PM Roman Gushchin <guro@fb.com> wrote: > > > > > > Include memory used by bpf programs into the memcg-based accounting. > > > This includes the memory used by programs itself, auxiliary data > > > and statistics. > > > > > > Signed-off-by: Roman Gushchin <guro@fb.com> > > > --- > > > kernel/bpf/core.c | 8 ++++---- > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c > > > index bde93344164d..daab8dcafbd4 100644 > > > --- a/kernel/bpf/core.c > > > +++ b/kernel/bpf/core.c > > > @@ -77,7 +77,7 @@ void *bpf_internal_load_pointer_neg_helper(const struct sk_buff *skb, int k, uns > > > > > > struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flags) > > > { > > > - gfp_t gfp_flags = GFP_KERNEL | __GFP_ZERO | gfp_extra_flags; > > > + gfp_t gfp_flags = GFP_KERNEL_ACCOUNT | __GFP_ZERO | gfp_extra_flags; > > > struct bpf_prog_aux *aux; > > > struct bpf_prog *fp; > > > > > > @@ -86,7 +86,7 @@ struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flag > > > if (fp == NULL) > > > return NULL; > > > > > > - aux = kzalloc(sizeof(*aux), GFP_KERNEL | gfp_extra_flags); > > > + aux = kzalloc(sizeof(*aux), GFP_KERNEL_ACCOUNT | gfp_extra_flags); > > > if (aux == NULL) { > > > vfree(fp); > > > return NULL; > > > @@ -104,7 +104,7 @@ struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flag > > > > > > struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags) > > > { > > > - gfp_t gfp_flags = GFP_KERNEL | __GFP_ZERO | gfp_extra_flags; > > > + gfp_t gfp_flags = GFP_KERNEL_ACCOUNT | __GFP_ZERO | gfp_extra_flags; > > > struct bpf_prog *prog; > > > int cpu; > > > > > > @@ -217,7 +217,7 @@ void bpf_prog_free_linfo(struct bpf_prog *prog) > > > struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size, > > > gfp_t gfp_extra_flags) > > > { > > > - gfp_t gfp_flags = GFP_KERNEL | __GFP_ZERO | gfp_extra_flags; > > > + gfp_t gfp_flags = GFP_KERNEL_ACCOUNT | __GFP_ZERO | gfp_extra_flags; > > > struct bpf_prog *fp; > > > u32 pages, delta; > > > int ret; > > > -- > > Hi Song! > > Thank you for looking into the patchset! > > > > > Do we need similar changes in > > > > bpf_prog_array_copy() > > bpf_prog_alloc_jited_linfo() > > bpf_prog_clone_create() > > > > and maybe a few more? > > I've tried to follow the rlimit-based accounting, so those objects which were > skipped are mostly skipped now and vice versa. The main reason for that is > simple: I don't know many parts of bpf code well enough to decide whether > we need accounting or not. > > In general with memcg-based accounting we can easily cover places which were > not covered previously: e.g. the memory used by the verifier. But I guess it's > better to do it case-by-case. > > But if you're aware of any big objects which should be accounted for sure, > please, let me know. Thanks for the explanation. I think we can do one-to-one migration to memcg-based accounting for now. Song
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c index bde93344164d..daab8dcafbd4 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -77,7 +77,7 @@ void *bpf_internal_load_pointer_neg_helper(const struct sk_buff *skb, int k, uns struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flags) { - gfp_t gfp_flags = GFP_KERNEL | __GFP_ZERO | gfp_extra_flags; + gfp_t gfp_flags = GFP_KERNEL_ACCOUNT | __GFP_ZERO | gfp_extra_flags; struct bpf_prog_aux *aux; struct bpf_prog *fp; @@ -86,7 +86,7 @@ struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flag if (fp == NULL) return NULL; - aux = kzalloc(sizeof(*aux), GFP_KERNEL | gfp_extra_flags); + aux = kzalloc(sizeof(*aux), GFP_KERNEL_ACCOUNT | gfp_extra_flags); if (aux == NULL) { vfree(fp); return NULL; @@ -104,7 +104,7 @@ struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flag struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags) { - gfp_t gfp_flags = GFP_KERNEL | __GFP_ZERO | gfp_extra_flags; + gfp_t gfp_flags = GFP_KERNEL_ACCOUNT | __GFP_ZERO | gfp_extra_flags; struct bpf_prog *prog; int cpu; @@ -217,7 +217,7 @@ void bpf_prog_free_linfo(struct bpf_prog *prog) struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size, gfp_t gfp_extra_flags) { - gfp_t gfp_flags = GFP_KERNEL | __GFP_ZERO | gfp_extra_flags; + gfp_t gfp_flags = GFP_KERNEL_ACCOUNT | __GFP_ZERO | gfp_extra_flags; struct bpf_prog *fp; u32 pages, delta; int ret;
Include memory used by bpf programs into the memcg-based accounting. This includes the memory used by programs itself, auxiliary data and statistics. Signed-off-by: Roman Gushchin <guro@fb.com> --- kernel/bpf/core.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)