| Message ID | 307f8b80-fca2-d55f-6bc6-85b794afa15e@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18.04.2018 20:21, Farhan Ali wrote: > On 04/18/2018 08:31 AM, Thomas Huth wrote: >> Some patches to improve the network boot experience on s390x: >> >> First, make sure that we shut down the virtio-net device before jumping >> into the kernel. Otherwise some incoming packets might destroy some of >> the kernel's data if it has not taken over the device yet. >> >> Then the last two patches add support for loading kernels via >> configuration files - pxelinux-style and .INS-file style. This way >> you don't have to manually glue your ramdisk to your kernel anymore, >> so this should be quite a relieve for all users who want to boot >> Linux via the network. >> >> The config file parsers have been completely written by myself from >> scratch and only tested with some config files that I came up with >> on my own. So if anybody has some pre-existing pxelinux config files >> already for booting a s390x, I'd appreciate some testing to see whether >> this works as expected for you, too! [...] > > I have tried with pxelinux default config file I had and it worked fine. > I will try a couple more tests. Thanks! > Also while going through the code again, I noticed a memory leak in the > function virtio_net_init, and fixed it. I could send a formal patch for > it, if you want to queue it as part of this series as I think we might > have missed 2.12 window? > > diff --git a/pc-bios/s390-ccw/virtio-net.c b/pc-bios/s390-ccw/virtio-net.c > index ff7f4da..e83ba08 100644 > --- a/pc-bios/s390-ccw/virtio-net.c > +++ b/pc-bios/s390-ccw/virtio-net.c > @@ -61,6 +61,7 @@ int virtio_net_init(void *mac_addr) > IPL_assert(buf != NULL, "Can not allocate memory for receive > buffers"); > vring_send_buf(rxvq, buf, ETH_MTU_SIZE + sizeof(VirtioNetHdr), > VRING_DESC_F_WRITE); > + free(buf); > } > vring_notify(rxvq); Ah, that's not a memory leak, though it might look like one if you don't know what the vring_send_buf function is doing: vring_send_buf adds a buffer to a virtio ring for later use. So the buffer is not unused after this function, but gets filled in later by the virtio-net device. Thus we must not free the buffer here. We could free it in the "uninit" function after shutting down the device, but OTOH we are then leaving the firmware code anyway by jumping into the OS kernel, so it does not really matter whether we free()ed the buffers or not. Thomas
On 04/19/2018 01:27 AM, Thomas Huth wrote: > Ah, that's not a memory leak, though it might look like one if you don't > know what the vring_send_buf function is doing: vring_send_buf adds a > buffer to a virtio ring for later use. So the buffer is not unused after > this function, but gets filled in later by the virtio-net device. Thus > we must not free the buffer here. > > We could free it in the "uninit" function after shutting down the > device, but OTOH we are then leaving the firmware code anyway by jumping > into the OS kernel, so it does not really matter whether we free()ed the > buffers or not. > > Thomas That makes a lot of sense now :) thanks a lot for that explanation and sorry for my ignorance.
diff --git a/pc-bios/s390-ccw/virtio-net.c b/pc-bios/s390-ccw/virtio-net.c index ff7f4da..e83ba08 100644 --- a/pc-bios/s390-ccw/virtio-net.c +++ b/pc-bios/s390-ccw/virtio-net.c @@ -61,6 +61,7 @@ int virtio_net_init(void *mac_addr) IPL_assert(buf != NULL, "Can not allocate memory for receive buffers"); vring_send_buf(rxvq, buf, ETH_MTU_SIZE + sizeof(VirtioNetHdr), VRING_DESC_F_WRITE); + free(buf); } vring_notify(rxvq);