From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1c9ZBe-00041A-6N for mharc-grub-devel@gnu.org; Wed, 23 Nov 2016 10:10:14 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56823) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c9ZBW-0003wT-IC for grub-devel@gnu.org; Wed, 23 Nov 2016 10:10:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c9ZBP-0003Bp-MS for grub-devel@gnu.org; Wed, 23 Nov 2016 10:10:06 -0500 Received: from userp1040.oracle.com ([156.151.31.81]:31019) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c9ZBP-0003AF-AA for grub-devel@gnu.org; Wed, 23 Nov 2016 10:09:59 -0500 Received: from aserv0022.oracle.com (aserv0022.oracle.com [141.146.126.234]) by userp1040.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with ESMTP id uANF9tvW015004 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Wed, 23 Nov 2016 15:09:56 GMT Received: from userv0122.oracle.com (userv0122.oracle.com [156.151.31.75]) by aserv0022.oracle.com (8.14.4/8.14.4) with ESMTP id uANF9tNq026428 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Wed, 23 Nov 2016 15:09:55 GMT Received: from abhmp0001.oracle.com (abhmp0001.oracle.com [141.146.116.7]) by userv0122.oracle.com (8.14.4/8.14.4) with ESMTP id uANF9sD8020960 for ; Wed, 23 Nov 2016 15:09:55 GMT Received: from [10.162.81.122] (/10.162.81.122) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 23 Nov 2016 07:09:54 -0800 Subject: Re: [PATCH 2/2] ofnet: implement a receive buffer To: The development of GNU GRUB , vasily.isaenko@oracle.com 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> <582F0234.1080400@oracle.com> <20161121214803.GD22877@router-fw-old.local.net-space.pl> <58345159.3030709@oracle.com> <20161123111654.GA23218@router-fw-old.local.net-space.pl> From: Stanislav Kholmanskikh Message-ID: <5835B140.60804@oracle.com> Date: Wed, 23 Nov 2016 18:09:52 +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: <20161123111654.GA23218@router-fw-old.local.net-space.pl> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Source-IP: aserv0022.oracle.com [141.146.126.234] X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.4.x-2.6.x [generic] [fuzzy] X-Received-From: 156.151.31.81 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: Wed, 23 Nov 2016 15:10:13 -0000 On 11/23/2016 02:16 PM, Daniel Kiper wrote: > On Tue, Nov 22, 2016 at 05:08:25PM +0300, Stanislav Kholmanskikh wrote: >> On 11/22/2016 12:48 AM, Daniel Kiper wrote: >>> On Fri, Nov 18, 2016 at 04:29:24PM +0300, Stanislav Kholmanskikh wrote: >>>> 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. >>> >>> Please do. >> >> Ok. I'll check what I can do here. > > Great! Thnaks! > >>>>>> 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. >>> >>> OK but if you move such code around anyway do not leave it unreadable. Improve it >>> by constants or comments. >> >> May I use a macro for this >> >> #define READ_TIMEOUT 200 >> >> ? > > It seems to me that it appears in one place, so, comment would be better here. > Unfortunately, it looks that there is no explanation for that value in commit > message... Ehhh... Could you check mail archive? If there is no such thing there > then let's leave it as it. Though I do not like it. Ok. I'll check the mail archive as well. > >>>>> 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. >>> >>> Great but why this numbers? >> >> I have to admit that I can't answer to your question. :( I copied this >> stuff from efi (for the receive buffer). The transmit buffer was already >> of this size. > > Ugh... Same as above... > > [...] > >>>>>> 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); >>> >>> It looks that it should be grub_zalloc() instead of grub_malloc() here. >> >> I have two reasons why I don't use grub_zalloc() here: >> >> 1. The buffer allocated with this function is written/read many times >> while grub is working. We write some amount of bytes to the buffer, and >> then read this amount of bytes. So I don't see why zeroing the buffer on > > I suppose that "this" == "same" here... > >> allocation should matter. >> >> 2. In IEEE1275-1994 I do not see an explicit notice that memory >> allocated with alloc-mem is zeroed. So for consistence of >> ofnet_alloc_netbuf() I do not call grub_zalloc() there. > > Yep, I am aware of that. However, I am asking because you change > currently exiting code behavior which uses grub_zalloc(). Maybe > we should leave it as is (I am thinking about grub_zalloc()) but > it is not very strong must. If you choose grub_malloc() please > explain in commit message why you did it and why it is safe here. Well, let it be grub_zalloc() then. > >>>>>> +} >>>>>> + >>>>>> +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; >>>>>> } >>> >>> Should not we free card->rcvbuf and/or card->txbuf if module is >>> unloaded or something like that? >> >> Yes, I think so. Thanks for pointing at this. >> >> It's interesting that none of uboot, efi, ieee1275 drivers seems to care >> of freeing the card data structure on module unload. All they do is >> similar to: >> >> FOR_NET_CARDS_SAFE (card, next) >> if (the card is handled by us) >> grub_net_card_unregister (card); >> >> whereas from grub-core/net/net.c I don't see that >> grub_net_card_unregister() frees memory. >> >> It seems that the job of freeing card's memory is expected to be handled >> in drivers and none of the drivers care about it, excluding pxe, where >> 'grub_pxe_card' is statically allocated. Or am I missing something? >> >> As for ieee1275 I'm thinking about something like: >> >> diff --git a/grub-core/net/drivers/ieee1275/ofnet.c >> b/grub-core/net/drivers/ieee1275/ofnet.c >> index 6bd3b92..12a4289 100644 >> --- a/grub-core/net/drivers/ieee1275/ofnet.c >> +++ b/grub-core/net/drivers/ieee1275/ofnet.c >> @@ -473,9 +473,28 @@ GRUB_MOD_INIT(ofnet) >> GRUB_MOD_FINI(ofnet) >> { >> struct grub_net_card *card, *next; >> + struct grub_ofnetcard_data *ofdata; >> >> FOR_NET_CARDS_SAFE (card, next) >> if (card->driver && grub_strcmp (card->driver->name, "ofnet") == 0) >> - grub_net_card_unregister (card); >> + { >> + grub_net_card_unregister (card); >> + >> + /* >> + * The fact that we are here means the card was successfully >> + * initialized in the past, so all the below pointers are valid, >> + * and we may free associated memory without checks. >> + */ >> + >> + ofdata = (struct grub_ofnetcard_data *) card->data; >> + grub_free (ofdata->path); >> + grub_free (ofdata->suffix); >> + grub_free (ofdata); >> + >> + ofnet_free_netbuf (card->txbuf, card->txbufsize); >> + ofnet_free_netbuf (card->rcvbuf, card->rcvbufsize); >> + >> + grub_free (card); >> + } >> grub_ieee1275_net_config = 0; >> } >> >> (not tested) >> >> I think it deserves a separate patch. In one patch we are adding the >> receive buffer, and in another we are making the ieee1275 driver to free >> all card resources on unload. > > Make sense for me. Could you do the same thing for other modules (at > least for those mentioned by you) too? Of course in separate patches. I'll do this for ieee1275. As for efi/uboot, I think I can also do this, but later, since testing this may take some time. I'd prefer to play with efi/uboot after finishing with this series :) Regarding this series. It seems we have all questions answered and the ball is on my side. I'll try to come up with V2 someday next week. Thank you for your time reviewing this! > > Daniel > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel >