| Message ID | 1424883057-12102-2-git-send-email-patila@marvell.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
Avinash Patil <patila@marvell.com> writes: > From: Marc Yang <yangyang@marvell.com> > > Current code does not check whether main_work_queue or > rx_work_queue is running when preparing to do queue_work, > this code fix add check before calling queue_work, reducing > unnecessary queue_work switch. > > This change instead sets more_task flag to ensure we run main_process > superloop once again. > > Signed-off-by: Marc Yang <yangyang@marvell.com> > Signed-off-by: Zhaoyang Liu <liuzy@marvell.com> > Signed-off-by: Shengzhen Li <szli@marvell.com> > Signed-off-by: Cathy Luo <cluo@marvell.com> > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com> > Signed-off-by: Avinash Patil <patila@marvell.com> Really, it took six persons to write this patch? Or do you just dump the same names to each patch? > +void mwifiex_queue_main_work(struct mwifiex_adapter *adapter) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&adapter->main_proc_lock, flags); > + if (adapter->mwifiex_processing) { > + adapter->more_task_flag = true; > + spin_unlock_irqrestore(&adapter->main_proc_lock, flags); > + } else { > + spin_unlock_irqrestore(&adapter->main_proc_lock, flags); > + queue_work(adapter->workqueue, &adapter->main_work); > + } > +} > +EXPORT_SYMBOL_GPL(mwifiex_queue_main_work); > + > +static void mwifiex_queue_rx_work(struct mwifiex_adapter *adapter) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&adapter->rx_proc_lock, flags); > + if (adapter->rx_processing) { > + spin_unlock_irqrestore(&adapter->rx_proc_lock, flags); > + } else { > + spin_unlock_irqrestore(&adapter->rx_proc_lock, flags); > + queue_work(adapter->rx_workqueue, &adapter->rx_work); > + } > +} I can apply this patch, but to me this looks like a horrible hack.
Hi Kalle, > -----Original Message----- > From: Kalle Valo [mailto:kvalo@codeaurora.org] > Sent: Tuesday, March 03, 2015 6:08 PM > To: Avinash Patil > Cc: linux-wireless@vger.kernel.org; Amitkumar Karwar; Cathy Luo; Zhaoyang Liu; > Shengzhen Li; Marc Yang > Subject: Re: [PATCH 1/4] mwifiex: avoid queue_work while work is ongoing > > Avinash Patil <patila@marvell.com> writes: > > > From: Marc Yang <yangyang@marvell.com> > > > > Current code does not check whether main_work_queue or rx_work_queue > > is running when preparing to do queue_work, this code fix add check > > before calling queue_work, reducing unnecessary queue_work switch. > > > > This change instead sets more_task flag to ensure we run main_process > > superloop once again. > > > > Signed-off-by: Marc Yang <yangyang@marvell.com> > > Signed-off-by: Zhaoyang Liu <liuzy@marvell.com> > > Signed-off-by: Shengzhen Li <szli@marvell.com> > > Signed-off-by: Cathy Luo <cluo@marvell.com> > > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com> > > Signed-off-by: Avinash Patil <patila@marvell.com> > > Really, it took six persons to write this patch? Or do you just dump the same > names to each patch? This should have been "Reviewed-by". Apologies. > > +void mwifiex_queue_main_work(struct mwifiex_adapter *adapter) { > > + unsigned long flags; > > + > > + spin_lock_irqsave(&adapter->main_proc_lock, flags); > > + if (adapter->mwifiex_processing) { > > + adapter->more_task_flag = true; > > + spin_unlock_irqrestore(&adapter->main_proc_lock, flags); > > + } else { > > + spin_unlock_irqrestore(&adapter->main_proc_lock, flags); > > + queue_work(adapter->workqueue, &adapter->main_work); > > + } > > +} > > +EXPORT_SYMBOL_GPL(mwifiex_queue_main_work); > > + > > +static void mwifiex_queue_rx_work(struct mwifiex_adapter *adapter) { > > + unsigned long flags; > > + > > + spin_lock_irqsave(&adapter->rx_proc_lock, flags); > > + if (adapter->rx_processing) { > > + spin_unlock_irqrestore(&adapter->rx_proc_lock, flags); > > + } else { > > + spin_unlock_irqrestore(&adapter->rx_proc_lock, flags); > > + queue_work(adapter->rx_workqueue, &adapter->rx_work); > > + } > > +} > > I can apply this patch, but to me this looks like a horrible hack. We are here trying to avoid requeueing another work when work item is being executed. We set more_task flag to true if work item is being executed and mwifiex_main_process would execute superloop once again if more_task it set. work_pending() cannot be of much help here since we want to avoid queing only when work item is being executed. Could you please let us know if you think of any better way to handle this? > -- > Kalle Valo -Avinash -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Avinash Patil <patila@marvell.com> writes: >> > From: Marc Yang <yangyang@marvell.com> >> > >> > Current code does not check whether main_work_queue or rx_work_queue >> > is running when preparing to do queue_work, this code fix add check >> > before calling queue_work, reducing unnecessary queue_work switch. >> > >> > This change instead sets more_task flag to ensure we run main_process >> > superloop once again. >> > >> > Signed-off-by: Marc Yang <yangyang@marvell.com> >> > Signed-off-by: Zhaoyang Liu <liuzy@marvell.com> >> > Signed-off-by: Shengzhen Li <szli@marvell.com> >> > Signed-off-by: Cathy Luo <cluo@marvell.com> >> > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com> >> > Signed-off-by: Avinash Patil <patila@marvell.com> >> >> Really, it took six persons to write this patch? Or do you just dump the same >> names to each patch? > > This should have been "Reviewed-by". Apologies. Ok, Reviewed-by would make more sense here. Please use that in the future (no need to change anything for these patches). >> > +void mwifiex_queue_main_work(struct mwifiex_adapter *adapter) { >> > + unsigned long flags; >> > + >> > + spin_lock_irqsave(&adapter->main_proc_lock, flags); >> > + if (adapter->mwifiex_processing) { >> > + adapter->more_task_flag = true; >> > + spin_unlock_irqrestore(&adapter->main_proc_lock, flags); >> > + } else { >> > + spin_unlock_irqrestore(&adapter->main_proc_lock, flags); >> > + queue_work(adapter->workqueue, &adapter->main_work); >> > + } >> > +} >> > +EXPORT_SYMBOL_GPL(mwifiex_queue_main_work); >> > + >> > +static void mwifiex_queue_rx_work(struct mwifiex_adapter *adapter) { >> > + unsigned long flags; >> > + >> > + spin_lock_irqsave(&adapter->rx_proc_lock, flags); >> > + if (adapter->rx_processing) { >> > + spin_unlock_irqrestore(&adapter->rx_proc_lock, flags); >> > + } else { >> > + spin_unlock_irqrestore(&adapter->rx_proc_lock, flags); >> > + queue_work(adapter->rx_workqueue, &adapter->rx_work); >> > + } >> > +} >> >> I can apply this patch, but to me this looks like a horrible hack. > > We are here trying to avoid requeueing another work when work item is > being executed. We set more_task flag to true if work item is being > executed and mwifiex_main_process would execute superloop once again > if more_task it set. work_pending() cannot be of much help here since > we want to avoid queing only when work item is being executed. Could > you please let us know if you think of any better way to handle this? It would be best to modify workqueue code to support this instead of doing this in the driver. But this was just an observation I made while reviewing your patches, I'll apply these anyway.
Hi Kalle, Please hold on. I am planning to divide patch3 into 2 patches - I will send updated v2. Thanks, Avinash
Avinash Patil <patila@marvell.com> writes: > Please hold on. > I am planning to divide patch3 into 2 patches - I will send updated v2. Ok, I'll wait for v2.
diff --git a/drivers/net/wireless/mwifiex/main.c b/drivers/net/wireless/mwifiex/main.c index 74488ab..447eb6f 100644 --- a/drivers/net/wireless/mwifiex/main.c +++ b/drivers/net/wireless/mwifiex/main.c @@ -131,6 +131,34 @@ static int mwifiex_unregister(struct mwifiex_adapter *adapter) return 0; } +void mwifiex_queue_main_work(struct mwifiex_adapter *adapter) +{ + unsigned long flags; + + spin_lock_irqsave(&adapter->main_proc_lock, flags); + if (adapter->mwifiex_processing) { + adapter->more_task_flag = true; + spin_unlock_irqrestore(&adapter->main_proc_lock, flags); + } else { + spin_unlock_irqrestore(&adapter->main_proc_lock, flags); + queue_work(adapter->workqueue, &adapter->main_work); + } +} +EXPORT_SYMBOL_GPL(mwifiex_queue_main_work); + +static void mwifiex_queue_rx_work(struct mwifiex_adapter *adapter) +{ + unsigned long flags; + + spin_lock_irqsave(&adapter->rx_proc_lock, flags); + if (adapter->rx_processing) { + spin_unlock_irqrestore(&adapter->rx_proc_lock, flags); + } else { + spin_unlock_irqrestore(&adapter->rx_proc_lock, flags); + queue_work(adapter->rx_workqueue, &adapter->rx_work); + } +} + static int mwifiex_process_rx(struct mwifiex_adapter *adapter) { unsigned long flags; @@ -154,7 +182,7 @@ static int mwifiex_process_rx(struct mwifiex_adapter *adapter) if (adapter->if_ops.submit_rem_rx_urbs) adapter->if_ops.submit_rem_rx_urbs(adapter); adapter->delay_main_work = false; - queue_work(adapter->workqueue, &adapter->main_work); + mwifiex_queue_main_work(adapter); } mwifiex_handle_rx_packet(adapter, skb); } @@ -214,9 +242,7 @@ process_start: if (atomic_read(&adapter->rx_pending) >= HIGH_RX_PENDING && adapter->iface_type != MWIFIEX_USB) { adapter->delay_main_work = true; - if (!adapter->rx_processing) - queue_work(adapter->rx_workqueue, - &adapter->rx_work); + mwifiex_queue_rx_work(adapter); break; } @@ -229,7 +255,7 @@ process_start: } if (adapter->rx_work_enabled && adapter->data_received) - queue_work(adapter->rx_workqueue, &adapter->rx_work); + mwifiex_queue_rx_work(adapter); /* Need to wake up the card ? */ if ((adapter->ps_state == PS_STATE_SLEEP) && @@ -606,7 +632,7 @@ int mwifiex_queue_tx_pkt(struct mwifiex_private *priv, struct sk_buff *skb) atomic_inc(&priv->adapter->tx_pending); mwifiex_wmm_add_buf_txqueue(priv, skb); - queue_work(priv->adapter->workqueue, &priv->adapter->main_work); + mwifiex_queue_main_work(priv->adapter); return 0; } diff --git a/drivers/net/wireless/mwifiex/main.h b/drivers/net/wireless/mwifiex/main.h index 16be45e..801a23b 100644 --- a/drivers/net/wireless/mwifiex/main.h +++ b/drivers/net/wireless/mwifiex/main.h @@ -1422,6 +1422,7 @@ u8 mwifiex_adjust_data_rate(struct mwifiex_private *priv, void mwifiex_dump_drv_info(struct mwifiex_adapter *adapter); void *mwifiex_alloc_rx_buf(int rx_len, gfp_t flags); +void mwifiex_queue_main_work(struct mwifiex_adapter *adapter); #ifdef CONFIG_DEBUG_FS void mwifiex_debugfs_init(void); diff --git a/drivers/net/wireless/mwifiex/pcie.c b/drivers/net/wireless/mwifiex/pcie.c index 4b463c3..c0ff33e 100644 --- a/drivers/net/wireless/mwifiex/pcie.c +++ b/drivers/net/wireless/mwifiex/pcie.c @@ -2101,7 +2101,7 @@ static irqreturn_t mwifiex_pcie_interrupt(int irq, void *context) goto exit; mwifiex_interrupt_status(adapter); - queue_work(adapter->workqueue, &adapter->main_work); + mwifiex_queue_main_work(adapter); exit: return IRQ_HANDLED; diff --git a/drivers/net/wireless/mwifiex/usb.c b/drivers/net/wireless/mwifiex/usb.c index 2238730..3adf1f6 100644 --- a/drivers/net/wireless/mwifiex/usb.c +++ b/drivers/net/wireless/mwifiex/usb.c @@ -193,7 +193,7 @@ static void mwifiex_usb_rx_complete(struct urb *urb) dev_dbg(adapter->dev, "info: recv_length=%d, status=%d\n", recv_length, status); if (status == -EINPROGRESS) { - queue_work(adapter->workqueue, &adapter->main_work); + mwifiex_queue_main_work(adapter); /* urb for data_ep is re-submitted now; * urb for cmd_ep will be re-submitted in callback @@ -262,7 +262,7 @@ static void mwifiex_usb_tx_complete(struct urb *urb) urb->status ? -1 : 0); } - queue_work(adapter->workqueue, &adapter->main_work); + mwifiex_queue_main_work(adapter); return; }