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
>>>>>>> <stanislav.kholmansk...@oracle.com>
>>>>>>> ---
>>>>>>> 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 <mrab...@br.ibm.com>
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 <phco...@gmail.com>
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
>
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
