[prev in list] [next in list] [prev in thread] [next in thread]
List: grub-devel
Subject: Re: [PATCH 2/2] ofnet: implement a receive buffer
From: Stanislav Kholmanskikh <stanislav.kholmanskikh () oracle ! com>
Date: 2016-11-30 15:27:44
Message-ID: 583EEFF0.6040006 () oracle ! com
[Download RAW message or body]
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.kholmanskikh@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 <mrabran@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 <phcoder@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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic