On 12/10/2010 08:12 PM, Greg KH wrote: > On Fri, Dec 10, 2010 at 08:00:49PM +0200, Timo Teräs wrote: >> On 12/10/2010 07:35 PM, Greg KH wrote: >>> On Fri, Dec 10, 2010 at 05:00:01PM +0200, Timo Teräs wrote: >>>> osd_schedule_callback() is called from VmbusOnMsgDPC() which runs >>>> in a tasklet. Avoid possible sleeping by using GFP_ATOMIC for the >>>> memory allocation. >>>> >>>> Seems to fix #16701. >>>> >>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=16701 >>>> Cc: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx> >>>> Cc: Hank Janssen <hjanssen@xxxxxxxxxxxxx> >>>> Signed-off-by: Timo Teräs <timo.teras@xxxxxx> >>>> --- >>>> drivers/staging/hv/osd.c | 2 +- >>>> 1 files changed, 1 insertions(+), 1 deletions(-) >>>> >>>> diff --git a/drivers/staging/hv/osd.c b/drivers/staging/hv/osd.c >>>> index 8c3eb27..eb9b20d 100644 >>>> --- a/drivers/staging/hv/osd.c >>>> +++ b/drivers/staging/hv/osd.c >>>> @@ -214,7 +214,7 @@ int osd_schedule_callback(struct workqueue_struct *wq, >>>> { >>>> struct osd_callback_struct *cb; >>>> >>>> - cb = kmalloc(sizeof(*cb), GFP_KERNEL); >>>> + cb = kmalloc(sizeof(*cb), GFP_ATOMIC); >>> >>> How about just fixing the one place where this is needed? You don't >>> need this for all of the osd_schedule_callback() functions, right? >>> >>> Actually, we should unroll this function entirely and use the "real" >>> functions the kernel provides us. I'd much rather see that fix than >>> this one which just papers over the problem. >> >> Yes, I agree. The osd_schedule_callback should definitely go away, we >> could also get rid of this kmalloc() by allocating the workqueue_struct >> as part of the callback context data which is the usual convention. > > Yes. > >> This was intended as minimum intrusion fix which would be suitable for >> -stable. As this bug seems to be a major issue in certain configurations. > > No, fix the bug properly now, we will worry about older kernel releases > later. > >> I'm not entirely sure about the usage of osd_schedule_callback() on >> other cases. It would appear that only VmbusOnMsgDPC() needs the atomic >> memory allocation. > > Then just "open code" the callback properly in that function to solve > the issue. > >> I preferred to not change the signature of an exported symbol. > > Why not? It's just for these few modules, no big dea. > >> I can resubmit a version which adds the gfp_mask as parameter to >> osd_schedule_callback if that would be acceptable for -stable. > > No, please fix it properly as described above. The VmbusOnMsgDPC() conversion is straight forward. See patch below (compile tested only). I'm uncertain if there could be multiple callbacks pending at the same time on the three other places. It would look like that: - free_channel() can happen only once - vmbus_onoffer() can happen only once - vmbus_onoffer_rescind() probably can happen only once, but not sure if it can be pending with either of the above ones. I think in all cases the struct work_struct can be embedded in struct vmbus_channel which is the context for the callback. Just not sure how many instances of it is needed. Staging: hv: convert VmbusOnMsgDPC() to not use osd_schedule_callback() The additional abstraction is unneeded. This also fixes a sleeping while atomic issue as osd_schedule_callback() can sleep which is not allowed for VmbusOnMsgDPC() running in a tasklet. Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=16701 Cc: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx> Cc: Hank Janssen <hjanssen@xxxxxxxxxxxxx> Signed-off-by: Timo Teräs <timo.teras@xxxxxx> --- index 45dbe30..820d529 100644 --- a/drivers/staging/hv/channel_mgmt.c +++ b/drivers/staging/hv/channel_mgmt.c @@ -736,9 +736,8 @@ static struct vmbus_channel_message_table_entry * * This is invoked in the vmbus worker thread context. */ -void vmbus_onmessage(void *context) +void vmbus_onmessage(struct hv_message *msg) { - struct hv_message *msg = context; struct vmbus_channel_message_header *hdr; int size; @@ -753,7 +752,6 @@ void vmbus_onmessage(void *context) hdr->MessageType, size); print_hex_dump_bytes("", DUMP_PREFIX_NONE, (unsigned char *)msg->u.Payload, size); - kfree(msg); return; } @@ -762,9 +760,6 @@ void vmbus_onmessage(void *context) else DPRINT_ERR(VMBUS, "Unhandled channel message type %d", hdr->MessageType); - - /* Free the msg that was allocated in VmbusOnMsgDPC() */ - kfree(msg); } /* diff --git a/drivers/staging/hv/channel_mgmt.h b/drivers/staging/hv/channel_mgmt.h index d16cc08..8293f6a 100644 --- a/drivers/staging/hv/channel_mgmt.h +++ b/drivers/staging/hv/channel_mgmt.h @@ -30,6 +30,7 @@ #include "ring_buffer.h" #include "vmbus_channel_interface.h" #include "vmbus_packet_format.h" +#include "hv_api.h" /* Version 1 messages */ enum vmbus_channel_message_type { @@ -309,7 +310,7 @@ struct vmbus_channel_msginfo { void free_channel(struct vmbus_channel *channel); -void vmbus_onmessage(void *context); +void vmbus_onmessage(struct hv_message *msg); int vmbus_request_offers(void); diff --git a/drivers/staging/hv/vmbus.c b/drivers/staging/hv/vmbus.c index d449daf..8607046 100644 --- a/drivers/staging/hv/vmbus.c +++ b/drivers/staging/hv/vmbus.c @@ -141,6 +141,21 @@ static void VmbusOnCleanup(struct hv_driver *drv) HvCleanup(); } +struct onmessage_work_context { + struct work_struct work; + struct hv_message msg; +}; + +static void vmbus_onmessage_work(struct work_struct *work) +{ + struct onmessage_work_context *ctx; + + ctx = container_of(work, struct onmessage_work_context, + work); + vmbus_onmessage(&ctx->msg); + kfree(ctx); +} + /* * VmbusOnMsgDPC - DPC routine to handle messages from the hypervisior */ @@ -150,20 +165,20 @@ static void VmbusOnMsgDPC(struct hv_driver *drv) void *page_addr = gHvContext.synICMessagePage[cpu]; struct hv_message *msg = (struct hv_message *)page_addr + VMBUS_MESSAGE_SINT; - struct hv_message *copied; + struct onmessage_work_context *ctx; while (1) { if (msg->Header.MessageType == HvMessageTypeNone) { /* no msg */ break; } else { - copied = kmemdup(msg, sizeof(*copied), GFP_ATOMIC); - if (copied == NULL) + ctx = kmalloc(sizeof(*ctx), GFP_ATOMIC); + if (ctx == NULL) continue; - osd_schedule_callback(gVmbusConnection.WorkQueue, - vmbus_onmessage, - (void *)copied); + INIT_WORK(&ctx->work, vmbus_onmessage_work); + memcpy(&ctx->msg, msg, sizeof(*msg)); + queue_work(gVmbusConnection.WorkQueue, &ctx->work); } msg->Header.MessageType = HvMessageTypeNone; _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
