> -----Original Message----- > From: Johannes Thumshirn [mailto:jthumshirn@xxxxxxx] > Sent: Monday, January 30, 2017 2:11 AM > To: Raghava Aditya Renukunta > <RaghavaAditya.Renukunta@xxxxxxxxxxxxx> > Cc: jejb@xxxxxxxxxxxxxxxxxx; martin.petersen@xxxxxxxxxx; linux- > scsi@xxxxxxxxxxxxxxx; Dave Carroll <david.carroll@xxxxxxxxxxxxx>; Gana > Sridaran <gana.sridaran@xxxxxxxxxxxxx>; Scott Benesh > <scott.benesh@xxxxxxxxxxxxx> > Subject: Re: [PATCH V3 10/24] aacraid: Reworked aac_command_thread > > EXTERNAL EMAIL > > > On Fri, Jan 27, 2017 at 11:28:39AM -0800, Raghava Aditya Renukunta wrote: > > Reworked aac_command_thread into aac_process_events > > > > Signed-off-by: Raghava Aditya Renukunta > <RaghavaAditya.Renukunta@xxxxxxxxxxxxx> > > Signed-off-by: Dave Carroll <David.Carroll@xxxxxxxxxxxxx> > > > > --- > > Wow, thanks for factoring this out. > > > +static void aac_process_events(struct aac_dev *dev) > > +{ > > + struct hw_fib *hw_fib, *hw_newfib; > > + struct fib *fib, *newfib; > > + struct aac_fib_context *fibctx; > > + unsigned long flags; > > + spinlock_t *t_lock; > > + > > t_lock = dev->queues->queue[HostNormCmdQueue].lock; > spin_lock_irqsave(t_lock, flags); Yes, let me change this peace of code. > > + spin_lock_irqsave(dev->queues->queue[HostNormCmdQueue].lock, > flags); > > + while (!list_empty(&(dev->queues- > >queue[HostNormCmdQueue].cmdq))) { > > + struct list_head *entry; > > + struct aac_aifcmd *aifcmd; > > + u32 time_now, time_last; > > + unsigned long flagv; > > + unsigned int num; > > + struct hw_fib **hw_fib_pool, **hw_fib_p; > > + struct fib **fib_pool, **fib_p; > > + > > + set_current_state(TASK_RUNNING); > > + > > + entry = dev->queues->queue[HostNormCmdQueue].cmdq.next; > > + list_del(entry); > > + > > + > > + t_lock = dev->queues->queue[HostNormCmdQueue].lock; > > + spin_unlock_irqrestore(t_lock, flags); > > + > > + fib = list_entry(entry, struct fib, fiblink); > > + hw_fib = fib->hw_fib_va; > > + /* > > + * We will process the FIB here or pass it to a > > + * worker thread that is TBD. We Really can't > > + * do anything at this point since we don't have > > + * anything defined for this thread to do. > > + */ > > + memset(fib, 0, sizeof(struct fib)); > > + fib->type = FSAFS_NTC_FIB_CONTEXT; > > + fib->size = sizeof(struct fib); > > + fib->hw_fib_va = hw_fib; > > + fib->data = hw_fib->data; > > + fib->dev = dev; > > + /* > > + * We only handle AifRequest fibs from the adapter. > > + */ > > + > > + aifcmd = (struct aac_aifcmd *) hw_fib->data; > > + if (aifcmd->command == cpu_to_le32(AifCmdDriverNotify)) { > > + /* Handle Driver Notify Events */ > > + aac_handle_aif(dev, fib); > > + *(__le32 *)hw_fib->data = cpu_to_le32(ST_OK); > > + aac_fib_adapter_complete(fib, (u16)sizeof(u32)); > > + continue; > > + } > > + /* > > + * The u32 here is important and intended. We are using > > + * 32bit wrapping time to fit the adapter field > > + */ > > + > > + > > + /* Sniff events */ > > + if ((aifcmd->command == cpu_to_le32(AifCmdEventNotify)) || > > + (aifcmd->command == > > + cpu_to_le32(AifCmdJobProgress))) { > > Parenthesis and indentation. Yes I will make the changes. > > > + aac_handle_aif(dev, fib); > > + } > > + > > + time_now = jiffies/HZ; > > + > > + /* > > + * Warning: no sleep allowed while > > + * holding spinlock. We take the estimate > > + * and pre-allocate a set of fibs outside the > > + * lock. > > + */ > > + num = le32_to_cpu(dev->init->r7.adapter_fibs_size) > > + / sizeof(struct hw_fib); /* some extra */ > > + spin_lock_irqsave(&dev->fib_lock, flagv); > > + entry = dev->fib_list.next; > > + while (entry != &dev->fib_list) { > > + entry = entry->next; > > + ++num; > > + } > > + spin_unlock_irqrestore(&dev->fib_lock, flagv); > > Bonus points for getting the estimation of the fibs into an own function. > From > the start of the comment till the spin_unlock(). Ok , let me do that. > > > + hw_fib_pool = kmalloc_array(num, sizeof(struct hw_fib *), > > + GFP_KERNEL); > > + fib_pool = kmalloc_array(num, sizeof(struct fib *), > > + GFP_KERNEL); > > + if (num && fib_pool && hw_fib_pool) { > > Ditto for the following block (this would have the benefit of describing what > the block does). Acknowledged > > > + hw_fib_p = hw_fib_pool; > > + fib_p = fib_pool; > > + while (hw_fib_p < &hw_fib_pool[num]) { > > + *(hw_fib_p) = kmalloc(sizeof(struct hw_fib), > > + GFP_KERNEL); > > + if (!(*(hw_fib_p++))) { > > + --hw_fib_p; > > + break; > > + } > > + *(fib_p) = kmalloc(sizeof(struct fib), > > + GFP_KERNEL); > > + if (!(*(fib_p++))) { > > + kfree(*(--hw_fib_p)); > > + break; > > + } > > + } > > + num = hw_fib_p - hw_fib_pool; > > + if (!num) { > > + kfree(fib_pool); > > + fib_pool = NULL; > > + kfree(hw_fib_pool); > > + hw_fib_pool = NULL; > > + continue; > > + } > > + } else { > > + kfree(hw_fib_pool); > > + hw_fib_pool = NULL; > > + kfree(fib_pool); > > + fib_pool = NULL; > > + } > > And for either everything under the fib_lock or the while loop. Acknowledged. > > + spin_lock_irqsave(&dev->fib_lock, flagv); > > + entry = dev->fib_list.next; > > + /* > > + * For each Context that is on the > > + * fibctxList, make a copy of the > > + * fib, and then set the event to wake up the > > + * thread that is waiting for it. > > + */ > > + hw_fib_p = hw_fib_pool; > > + fib_p = fib_pool; > > + while (entry != &dev->fib_list) { > > + /* > > + * Extract the fibctx > > + */ > > + fibctx = list_entry(entry, struct aac_fib_context, > > + next); > > + /* > > + * Check if the queue is getting > > + * backlogged > > + */ > > + if (fibctx->count > 20) { > > + /* > > + * It's *not* jiffies folks, > > + * but jiffies / HZ so do not > > + * panic ... > > + */ > > + time_last = fibctx->jiffies; > > + /* > > + * Has it been > 2 minutes > > + * since the last read off > > + * the queue? > > + */ > > + if ((time_now - time_last) > aif_timeout) { > > + entry = entry->next; > > + aac_close_fib_context(dev, fibctx); > > + continue; > > + } > > + } > > + /* > > + * Warning: no sleep allowed while > > + * holding spinlock > > + */ > > + if (hw_fib_p < &hw_fib_pool[num]) { > > + hw_newfib = *hw_fib_p; > > + *(hw_fib_p++) = NULL; > > + newfib = *fib_p; > > + *(fib_p++) = NULL; > > + /* > > + * Make the copy of the FIB > > + */ > > + memcpy(hw_newfib, hw_fib, > > + sizeof(struct hw_fib)); > > + memcpy(newfib, fib, sizeof(struct fib)); > > + newfib->hw_fib_va = hw_newfib; > > + /* > > + * Put the FIB onto the > > + * fibctx's fibs > > + */ > > + list_add_tail(&newfib->fiblink, > > + &fibctx->fib_list); > > + fibctx->count++; > > + /* > > + * Set the event to wake up the > > + * thread that is waiting. > > + */ > > + up(&fibctx->wait_sem); > > + } else { > > + pr_warn("aifd: didn't allocate NewFib.\n"); > > + } > > + entry = entry->next; > > + } > > + /* > > + * Set the status of this FIB > > + */ > > + *(__le32 *)hw_fib->data = cpu_to_le32(ST_OK); > > + aac_fib_adapter_complete(fib, sizeof(u32)); > > + spin_unlock_irqrestore(&dev->fib_lock, flagv); > > + /* Free up the remaining resources */ > > + hw_fib_p = hw_fib_pool; > > + fib_p = fib_pool; > > + while (hw_fib_p < &hw_fib_pool[num]) { > > + kfree(*hw_fib_p); > > + kfree(*fib_p); > > + ++fib_p; > > + ++hw_fib_p; > > + } > > + kfree(hw_fib_pool); > > + kfree(fib_pool); > > + kfree(fib); > > + spin_lock_irqsave(dev->queues- > >queue[HostNormCmdQueue].lock, > > + flags); > > + } > > + /* > > + * There are no more AIF's > > + */ > > + spin_unlock_irqrestore(dev->queues- > >queue[HostNormCmdQueue].lock, > > + flags); > > +} > > [...] > > Thanks, > this change is highly appreciated by me > > Johannes > > -- > Johannes Thumshirn Storage > jthumshirn@xxxxxxx +49 911 74053 689 > SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg > GF: Felix Imendörffer, Jane Smithard, Graham Norton > HRB 21284 (AG Nürnberg) > Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
