From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1c7jEX-0005a3-5m for mharc-grub-devel@gnu.org; Fri, 18 Nov 2016 08:29:37 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33004) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c7jEU-0005Zs-BT for grub-devel@gnu.org; Fri, 18 Nov 2016 08:29:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c7jER-0008NN-1S for grub-devel@gnu.org; Fri, 18 Nov 2016 08:29:34 -0500 Received: from aserp1040.oracle.com ([141.146.126.69]:22295) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c7jEQ-0008N4-Pp for grub-devel@gnu.org; Fri, 18 Nov 2016 08:29:30 -0500 Received: from userv0022.oracle.com (userv0022.oracle.com [156.151.31.74]) by aserp1040.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with ESMTP id uAIDTRXZ021164 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Fri, 18 Nov 2016 13:29:28 GMT Received: from userv0122.oracle.com (userv0122.oracle.com [156.151.31.75]) by userv0022.oracle.com (8.14.4/8.14.4) with ESMTP id uAIDTRa1022514 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Fri, 18 Nov 2016 13:29:27 GMT Received: from abhmp0005.oracle.com (abhmp0005.oracle.com [141.146.116.11]) by userv0122.oracle.com (8.14.4/8.14.4) with ESMTP id uAIDTRtr023098 for ; Fri, 18 Nov 2016 13:29:27 GMT Received: from [10.162.81.122] (/10.162.81.122) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Fri, 18 Nov 2016 05:29:26 -0800 Subject: Re: [PATCH 2/2] ofnet: implement a receive buffer To: The development of GNU GRUB References: <1460464796-24738-1-git-send-email-stanislav.kholmanskikh@oracle.com> <1460464796-24738-2-git-send-email-stanislav.kholmanskikh@oracle.com> <20161115223417.GI16470@router-fw-old.local.net-space.pl> Cc: vasily.isaenko@oracle.com From: Stanislav Kholmanskikh Message-ID: <582F0234.1080400@oracle.com> Date: Fri, 18 Nov 2016 16:29:24 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <20161115223417.GI16470@router-fw-old.local.net-space.pl> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Source-IP: userv0022.oracle.com [156.151.31.74] X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.4.x-2.6.x [generic] [fuzzy] X-Received-From: 141.146.126.69 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: Fri, 18 Nov 2016 13:29:36 -0000 On 11/16/2016 01:34 AM, Daniel Kiper wrote: > 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. No. I didn't do performance testing. > >> 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. The whole 'do while' construction is from the existing code, I only modify the destination for the grub_ieee1275_read() call. > > Additionally, are we sure that whole packet can be always stored in dev->rcvbuf? Code in search_net_devices() allocates the buffer to be of size: ALIGN_UP (card->mtu, 64) + 256; so, yes, it's capable to handle any valid packet size. > >> + 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. Ok. I can move this change into a separate 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... Ok. > > 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... No. Patch 1 is about two supplementary funcions for "alloc-mem", "free-mem". The changes below setup the transmit/receive buffers for a network card. The changes above use this receive buffer. So, in my opinion, this all is logically coupled and should be in one patch. > >> 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 > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel >