From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1c6mJ8-0005sO-IV for mharc-grub-devel@gnu.org; Tue, 15 Nov 2016 17:34:27 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47266) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6mJ5-0005r7-B9 for grub-devel@gnu.org; Tue, 15 Nov 2016 17:34:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c6mJ2-0000w4-4v for grub-devel@gnu.org; Tue, 15 Nov 2016 17:34:23 -0500 Received: from boksu.net-space.pl ([185.15.1.105]:53493) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:24) (Exim 4.71) (envelope-from ) id 1c6mJ1-0000v6-Q6 for grub-devel@gnu.org; Tue, 15 Nov 2016 17:34:20 -0500 Received: (from localhost user: 'dkiper' uid#4000 fake: STDIN (dkiper@boksu.net-space.pl)) by router-fw-old.local.net-space.pl id S1667377AbcKOWeR (ORCPT ); Tue, 15 Nov 2016 23:34:17 +0100 Date: Tue, 15 Nov 2016 23:34:17 +0100 From: Daniel Kiper To: stanislav.kholmanskikh@oracle.com, grub-devel@gnu.org Cc: vasily.isaenko@oracle.com Subject: Re: [PATCH 2/2] ofnet: implement a receive buffer Message-ID: <20161115223417.GI16470@router-fw-old.local.net-space.pl> References: <1460464796-24738-1-git-send-email-stanislav.kholmanskikh@oracle.com> <1460464796-24738-2-git-send-email-stanislav.kholmanskikh@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1460464796-24738-2-git-send-email-stanislav.kholmanskikh@oracle.com> User-Agent: Mutt/1.3.28i X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 185.15.1.105 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 15 Nov 2016 22:34:24 -0000 On Tue, Apr 12, 2016 at 03:39:56PM +0300, Stanislav Kholmanskikh wrote: > get_card_packet() from ofnet.c allocates a netbuff based on the device's MTU: > > nb = grub_netbuff_alloc (dev->mtu + 64 + 2); > > In the case when the MTU is large, and the received packet is > relatively small, this leads to allocation of significantly more memory, > than it's required. An example could be transmission of TFTP packets > with 0x400 blksize via a network card with 0x10000 MTU. > > This patch implements a per-card receive buffer in a way similar to efinet.c, > and makes get_card_packet() allocate a netbuff of the received data size. Have you checked performance impact of this patch? It should not be meaningful but it is good to know. > Signed-off-by: Stanislav Kholmanskikh > --- > grub-core/net/drivers/ieee1275/ofnet.c | 100 ++++++++++++++++++------------- > 1 files changed, 58 insertions(+), 42 deletions(-) > > diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c > index 6bd3b92..09ec77e 100644 > --- a/grub-core/net/drivers/ieee1275/ofnet.c > +++ b/grub-core/net/drivers/ieee1275/ofnet.c > @@ -85,24 +85,35 @@ get_card_packet (struct grub_net_card *dev) > grub_uint64_t start_time; > struct grub_net_buff *nb; > > - nb = grub_netbuff_alloc (dev->mtu + 64 + 2); > + start_time = grub_get_time_ms (); > + do > + rc = grub_ieee1275_read (data->handle, dev->rcvbuf, dev->rcvbufsize, &actual); > + while ((actual <= 0 || rc < 0) && (grub_get_time_ms () - start_time < 200)); Why 200? Please avoid using plain numbers if possible. Use constants. If it does not make sense then put comment which explains why this figure not another. Additionally, are we sure that whole packet can be always stored in dev->rcvbuf? > + if (actual <= 0) > + return NULL; > + > + nb = grub_netbuff_alloc (actual + 2); > if (!nb) > return NULL; > + > /* Reserve 2 bytes so that 2 + 14/18 bytes of ethernet header is divisible > by 4. So that IP header is aligned on 4 bytes. */ > - grub_netbuff_reserve (nb, 2); > + if (grub_netbuff_reserve (nb, 2)) > + { > + grub_netbuff_free (nb); > + return NULL; > + } This smells like separate fix not belonging to this patch. > - start_time = grub_get_time_ms (); > - do > - rc = grub_ieee1275_read (data->handle, nb->data, dev->mtu + 64, &actual); > - while ((actual <= 0 || rc < 0) && (grub_get_time_ms () - start_time < 200)); > - if (actual > 0) > + grub_memcpy (nb->data, dev->rcvbuf, actual); > + > + if (grub_netbuff_put (nb, actual)) > { > - grub_netbuff_put (nb, actual); > - return nb; > + grub_netbuff_free (nb); > + return NULL; > } Why not... if (!grub_netbuff_put (nb, actual)) return nb; > - grub_netbuff_free (nb); > - return NULL; > + > + return nb; ...then you do not need these changes too... > } It looks that everything below belongs to patch #1... > static struct grub_net_card_driver ofdriver = > @@ -294,6 +305,24 @@ grub_ieee1275_net_config_real (const char *devpath, char **device, char **path, > } > } > > +static void * > +ofnet_alloc_netbuf (grub_size_t len) > +{ > + if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN)) > + return grub_ieee1275_alloc_mem (len); > + else > + return grub_malloc (len); > +} > + > +static void > +ofnet_free_netbuf (void *addr, grub_size_t len) > +{ > + if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN)) > + grub_ieee1275_free_mem (addr, len); > + else > + grub_free (addr); > +} > + > static int > search_net_devices (struct grub_ieee1275_devalias *alias) > { > @@ -409,41 +438,21 @@ search_net_devices (struct grub_ieee1275_devalias *alias) > card->default_address = lla; > > card->txbufsize = ALIGN_UP (card->mtu, 64) + 256; > + card->rcvbufsize = ALIGN_UP (card->mtu, 64) + 256; > > - if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN)) > - { > - struct alloc_args > - { > - struct grub_ieee1275_common_hdr common; > - grub_ieee1275_cell_t method; > - grub_ieee1275_cell_t len; > - grub_ieee1275_cell_t catch; > - grub_ieee1275_cell_t result; > - } > - args; > - INIT_IEEE1275_COMMON (&args.common, "interpret", 2, 2); > - args.len = card->txbufsize; > - args.method = (grub_ieee1275_cell_t) "alloc-mem"; > - > - if (IEEE1275_CALL_ENTRY_FN (&args) == -1 > - || args.catch) > - { > - card->txbuf = 0; > - grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory")); > - } > - else > - card->txbuf = (void *) args.result; > - } > - else > - card->txbuf = grub_zalloc (card->txbufsize); > + card->txbuf = ofnet_alloc_netbuf (card->txbufsize); > if (!card->txbuf) > + goto fail; > + > + card->rcvbuf = ofnet_alloc_netbuf (card->rcvbufsize); > + if (!card->rcvbuf) > { > - grub_free (ofdata->path); > - grub_free (ofdata); > - grub_free (card); > - grub_print_error (); > - return 0; > + grub_error_push (); > + ofnet_free_netbuf(card->txbuf, card->txbufsize); > + grub_error_pop (); > + goto fail; > } > + > card->driver = NULL; > card->data = ofdata; > card->flags = 0; > @@ -455,6 +464,13 @@ search_net_devices (struct grub_ieee1275_devalias *alias) > card->driver = &ofdriver; > grub_net_card_register (card); > return 0; > + > + fail: > + grub_free (ofdata->path); > + grub_free (ofdata); > + grub_free (card); > + grub_print_error (); > + return 1; ...and without full explanation why in #1 commit message it is not obvious for what this change is really needed... Daniel