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