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

[Daniel Kiper]

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.

> >> 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.

> > 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?

> >> +  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.

Thanks a lot!

> >> -  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.

I have checked code below once again. First of all I think that
grub_ieee1275_alloc_mem() and grub_ieee1275_free_mem() have to live
in this file not in grub-core/kern/ieee1275/openfw.c. I see no
other callers for both of them. Additionally, patch #1 should
introduce grub_ieee1275_alloc_mem(), ofnet_alloc_netbuf() and
search_net_devices() should call ofnet_alloc_netbuf(). No more
no less. Do not forget to mention in commit message why patch #1
is needed. Then patch #2 should introduce rest of the code below.

> >>  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.

> >> +}
> >> +
> >> +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?

Daniel