From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1cC6ni-0001Zd-QK for mharc-grub-devel@gnu.org; Wed, 30 Nov 2016 10:28:02 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38608) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cC6nf-0001Y2-Sc for grub-devel@gnu.org; Wed, 30 Nov 2016 10:28:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cC6nc-00077K-Lr for grub-devel@gnu.org; Wed, 30 Nov 2016 10:27:59 -0500 Received: from userp1040.oracle.com ([156.151.31.81]:51907) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cC6nc-00076l-BF for grub-devel@gnu.org; Wed, 30 Nov 2016 10:27:56 -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 uAUFRoTY010080 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Wed, 30 Nov 2016 15:27:52 GMT Received: from userv0121.oracle.com (userv0121.oracle.com [156.151.31.72]) by aserv0022.oracle.com (8.14.4/8.14.4) with ESMTP id uAUFRngm021638 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Wed, 30 Nov 2016 15:27:50 GMT Received: from abhmp0004.oracle.com (abhmp0004.oracle.com [141.146.116.10]) by userv0121.oracle.com (8.14.4/8.13.8) with ESMTP id uAUFRmms022812 for ; Wed, 30 Nov 2016 15:27:49 GMT Received: from [10.162.81.122] (/10.162.81.122) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 30 Nov 2016 07:27:48 -0800 Subject: Re: [PATCH 2/2] ofnet: implement a receive buffer Reply-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> <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> <5835B140.60804@oracle.com> To: The development of GNU GRUB , vasily.isaenko@oracle.com From: Stanislav Kholmanskikh Message-ID: <583EEFF0.6040006@oracle.com> Date: Wed, 30 Nov 2016 18:27:44 +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: <5835B140.60804@oracle.com> 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, 30 Nov 2016 15:28:01 -0000 On 11/23/2016 06:09 PM, Stanislav Kholmanskikh wrote: > > > 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. What I found is that this timeout of 200 ms was introduced by commit: commit 0f231af8ae44b6e4efe6b25794db21fbfd270718 Author: Manoel Rebelo Abranches Date: Tue May 10 09:50:18 2011 -0300 Implement timeout when receiving packets. It seems this commit has roots here: http://lists.gnu.org/archive/html/grub-devel/2011-05/msg00041.html where my search stops. > >> >>>>>> 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... It seems that commit: commit 3e7472395127dc231c0f7139600b0465f68d0095 Author: Vladimir 'phcoder' Serbinenko Date: Sat Jun 9 11:00:18 2012 +0200 Keep TX and RX buffers on EFI rather than always allocate new ones. * include/grub/net.h (grub_net_card_driver): Allow driver to modify card. All users updated. (grub_net_card): New members txbuf, rcvbuf, rcvbufsize and txbusy. * grub-core/net/drivers/efi/efinet.c (send_card_buffer): Reuse buffer. (get_card_packet): Likewise. (grub_efinet_findcards): Init new fields. was the first one to use ALIGN_UP (card->mtu, 64) + 256 for the receive buffer. Before that the buffer size was hard coded to 1536. I could not find an email message describing this change... >> >> [...] >> >>>>>>> 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 >> > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel >