Re: [PATCH V2 4/4] ofnet: implement the receive buffer

[Andrei Borzenkov <arvidjaar@gmail.com>]

02.12.2016 18:10, Stanislav Kholmanskikh пишет:
> 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.
> 
> Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
> ---
>  grub-core/net/drivers/ieee1275/ofnet.c |   50 ++++++++++++++++++++++---------
>  1 files changed, 35 insertions(+), 15 deletions(-)
> 
> diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
> index 1f8ac9a..471b87b 100644
> --- a/grub-core/net/drivers/ieee1275/ofnet.c
> +++ b/grub-core/net/drivers/ieee1275/ofnet.c
> @@ -85,9 +85,18 @@ 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));
> +
> +  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. */
>    if (grub_netbuff_reserve (nb, 2))
> @@ -96,17 +105,15 @@ get_card_packet (struct grub_net_card *dev)
>        return NULL;
>      }
>  
> -  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;
>      }
> -  grub_netbuff_free (nb);
> -  return NULL;
> +
> +  return nb;
>  }
>  
>  static struct grub_net_card_driver ofdriver =
> @@ -498,16 +505,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;
>  
>    card->txbuf = ofnet_alloc_netbuf (card->txbufsize);
>    if (!card->txbuf)
> +    goto fail_netbuf;
> +
> +  card->rcvbuf = ofnet_alloc_netbuf (card->rcvbufsize);
> +  if (!card->rcvbuf)
>      {
> -      grub_free (ofdata->path);
> -      grub_free (ofdata);
> -      grub_free (card);
> -      grub_print_error ();
> -      return 1;
> +      grub_error_push ();
> +      ofnet_free_netbuf(card->txbuf, card->txbufsize);
> +      grub_error_pop ();
> +      goto fail_netbuf;
>      }
> +  
>    card->driver = NULL;
>    card->data = ofdata;
>    card->flags = 0;
> @@ -519,6 +531,13 @@ search_net_devices (struct grub_ieee1275_devalias *alias)
>    card->driver = &ofdriver;
>    grub_net_card_register (card);
>    return 0;
> +
> +fail_netbuf:
> +  grub_free (ofdata->path);
> +  grub_free (ofdata);
> +  grub_free (card);
> +  grub_print_error ();
> +  return 1;
>  }
>  
>  static void
> @@ -553,6 +572,7 @@ GRUB_MOD_FINI(ofnet)
>  	grub_free (ofdata);
>  
>  	ofnet_free_netbuf (card->txbuf, card->txbufsize);
> +	ofnet_free_netbuf (card->rcvbuf, card->rcvbufsize);
>  
>  	grub_free ((void *) card->name);
>  	grub_free (card);
> 

See comment to 3/4. Otherwise OK from my side.