From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S967458AbcA0UDW (ORCPT ); Wed, 27 Jan 2016 15:03:22 -0500 Received: from mail-pa0-f67.google.com ([209.85.220.67]:32944 "EHLO mail-pa0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S967185AbcA0UDI (ORCPT ); Wed, 27 Jan 2016 15:03:08 -0500 Message-ID: <56A92246.7010807@gmail.com> Date: Wed, 27 Jan 2016 12:02:14 -0800 From: Florian Fainelli User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 To: Gregory CLEMENT , "David S. Miller" , linux-kernel@vger.kernel.org, netdev@vger.kernel.org, Thomas Petazzoni CC: Jason Cooper , Andrew Lunn , Sebastian Hesselbarth , linux-arm-kernel@lists.infradead.org, Lior Amsalem , Nadav Haklai , Marcin Wojtas , Simon Guinot , Ezequiel Garcia , Maxime Ripard , Boris BREZILLON , Russell King - ARM Linux , Willy Tarreau , Arnd Bergmann Subject: Re: [PATCH net-next 09/10] net: Add a hardware buffer management helper API References: <1452625834-22166-1-git-send-email-gregory.clement@free-electrons.com> <1452625834-22166-10-git-send-email-gregory.clement@free-electrons.com> In-Reply-To: <1452625834-22166-10-git-send-email-gregory.clement@free-electrons.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/01/16 11:10, Gregory CLEMENT wrote: > This basic implementation allows to share code between driver using > hardware buffer management. As the code is hardware agnostic, there is > few helpers, most of the optimization brought by the an HW BM has to be > done at driver level. > > Signed-off-by: Gregory CLEMENT > --- > include/net/hwbm.h | 19 +++++++++++++ > net/core/Makefile | 2 +- > net/core/hwbm.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 98 insertions(+), 1 deletion(-) > create mode 100644 include/net/hwbm.h > create mode 100644 net/core/hwbm.c > > diff --git a/include/net/hwbm.h b/include/net/hwbm.h > new file mode 100644 > index 000000000000..898ccd2fb58d > --- /dev/null > +++ b/include/net/hwbm.h > @@ -0,0 +1,19 @@ > +#ifndef _HWBM_H > +#define _HWBM_H > + > +struct hwbm_pool { > + /* Size of the buffers managed */ > + int size; > + /* Number of buffers currently used by this pool */ > + int buf_num; > + /* constructor called during alocation */ > + int (*construct)(struct hwbm_pool *bm_pool, void *buf); Having the buffer size might be handy too. > + /* private data */ > + void *priv; > +}; > + > +void hwbm_buf_free(struct hwbm_pool *bm_pool, void *buf); > +int hwbm_pool_refill(struct hwbm_pool *bm_pool); > +int hwbm_pool_add(struct hwbm_pool *bm_pool, int buf_num); > + > +#endif /* _HWBM_H */ > diff --git a/net/core/Makefile b/net/core/Makefile > index 0b835de04de3..df81bf11f072 100644 > --- a/net/core/Makefile > +++ b/net/core/Makefile > @@ -9,7 +9,7 @@ obj-$(CONFIG_SYSCTL) += sysctl_net_core.o > > obj-y += dev.o ethtool.o dev_addr_lists.o dst.o netevent.o \ > neighbour.o rtnetlink.o utils.o link_watch.o filter.o \ > - sock_diag.o dev_ioctl.o tso.o sock_reuseport.o > + sock_diag.o dev_ioctl.o tso.o sock_reuseport.o hwbm.o Not everybody will want this built in by default, we probably need a hidden config symbol here. > > obj-$(CONFIG_XFRM) += flow.o > obj-y += net-sysfs.o > diff --git a/net/core/hwbm.c b/net/core/hwbm.c > new file mode 100644 > index 000000000000..d5d40d63cb34 > --- /dev/null > +++ b/net/core/hwbm.c > @@ -0,0 +1,78 @@ > +/* Support for hardware buffer manager. > + * > + * Copyright (C) 2016 Marvell > + * > + * Gregory CLEMENT > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > +#include > +#include > +#include > +#include > + > +void hwbm_buf_free(struct hwbm_pool *bm_pool, void *buf) > +{ > + if (likely(bm_pool->size <= PAGE_SIZE)) > + skb_free_frag(buf); > + else > + kfree(buf); > +} > +EXPORT_SYMBOL_GPL(hwbm_buf_free); > + > +/* Refill processing for HW buffer management */ > +int hwbm_pool_refill(struct hwbm_pool *bm_pool) > +{ > + void *buf; > + int frag_size = bm_pool->size; Reverse christmas tree declaration looks a bit nicer. > + > + if (likely(frag_size <= PAGE_SIZE)) > + buf = netdev_alloc_frag(frag_size); > + else > + buf = kmalloc(frag_size, GFP_ATOMIC); Maybe we should allow the caller to specify a gfp_t, just in case GFP_ATOMIC is not good enough. > + > + if (!buf) > + return -ENOMEM; > + > + if (bm_pool->construct) > + if (bm_pool->construct(bm_pool, buf)) { > + hwbm_buf_free(bm_pool, buf); > + return -ENOMEM; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(hwbm_pool_refill); > + > +int hwbm_pool_add(struct hwbm_pool *bm_pool, int buf_num) unsigned int buf_num > +{ > + int err, i; > + > + if (bm_pool->buf_num == bm_pool->size) { > + pr_debug("pool already filled\n"); > + return bm_pool->buf_num; > + } > + > + if (buf_num + bm_pool->buf_num > bm_pool->size) { > + pr_debug("cannot allocate %d buffers for pool\n", > + buf_num); > + return 0; > + } buf_num is under caller control, and potentially hardware control indirectly, what if I make this arbitrary big and wrap around? > + > + for (i = 0; i < buf_num; i++) { > + err = hwbm_pool_refill(bm_pool); > + if (err < 0) > + break; > + } If we fail refiling here, should not we propagate the error back to the caller? > + > + /* Update BM driver with number of buffers added to pool */ > + bm_pool->buf_num += i; > + > + pr_debug("hwpm pool: %d of %d buffers added\n", i, buf_num); No locking or atomic operations here? What if two CPUs call into this function? > + > + return i; > +} > +EXPORT_SYMBOL_GPL(hwbm_pool_add); > -- Florian