Re: [PATCH 2/2] ofnet: implement a receive buffer

[Stanislav Kholmanskikh]

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 <address@hidden>
>>>>>>> ---
>>>>>>>  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 <address@hidden>
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 <address@hidden>
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
>> address@hidden
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel
>