On Dec 4, 2012, at 6:39 PM, Andrew Morton wrote:
> On Mon, 3 Dec 2012 20:40:55 -0500
> Ed Cashin <ecas...@coraid.com> wrote:
...
>> +static void
>> +ata_rw_frameinit(struct frame *f)
>> +{
>> + struct aoetgt *t;
>> + struct aoe_hdr *h;
>> + struct aoe_atahdr *ah;
>> + struct sk_buff *skb;
>> + char writebit, extbit;
>> +
>> + skb = f->skb;
>> + h = (struct aoe_hdr *) skb_mac_header(skb);
>> + ah = (struct aoe_atahdr *) (h + 1);
>
> Well. It would be neater to have a
>
> struct whatever {
> struct aoe_hdr hdr;
> struct aoe_atahdr atahdr;
> };
This code just moved from aoecmd_ata_rw into a function that can be
used by both aoecmd_ata_rw() and the new probe() function, so this
"h+1" way of getting at the ATA header is not new. I'll make a note of
your recommendation for future new code and for cleanups.
...
>> @@ -462,11 +488,14 @@ resend(struct aoedev *d, struct frame *f)
>> h = (struct aoe_hdr *) skb_mac_header(skb);
>> ah = (struct aoe_atahdr *) (h+1);
>>
>> - snprintf(buf, sizeof buf,
>> - "%15s e%ld.%d oldtag=%08x@%08lx newtag=%08x s=%pm d=%pm
>> nout=%d\n",
>> - "retransmit", d->aoemajor, d->aoeminor, f->tag, jiffies, n,
>> - h->src, h->dst, t->nout);
>> - aoechr_error(buf);
>> + if (!(f->flags & FFL_PROBE)) {
>> + snprintf(buf, sizeof(buf),
>> + "%15s e%ld.%d oldtag=%08x@%08lx newtag=%08x s=%pm d=%pm
>> nout=%d\n",
>> + "retransmit", d->aoemajor, d->aoeminor,
>> + f->tag, jiffies, n,
>> + h->src, h->dst, t->nout);
>> + aoechr_error(buf);
>
> Could use kasprintf() here. That avoids the fixed-size local buffer
> and avoids the GFP_ATOMIC allocation and copy in aoechr_error().
This patch doesn't change the way aoechr_error() is used. But
thinking about the suggestion, I see that avoiding the GFP_ATOMIC
allocation isn't practical here, because resend() is called after
spin_lock_irq(). Let me know if you still think it would be worth
cleaning up. It could go on the todo list.
...
>> aoecmd_init(void)
>> {
>> + void *p;
>> +
>> + /* get_zeroed_page returns page with ref count 1 */
>> + p = (void *) get_zeroed_page(GFP_KERNEL | __GFP_REPEAT);
>> + if (!p)
>> + return -ENOMEM;
>> + empty_page = virt_to_page(p);
>
> Could use alloc_pages() and remove `p' and the virt_to_page().
>
> Why is __GFP_REPEAT used? I don't think this __init function is more
> important than all the other ones in the kernel?
I think you are not implying that it's hurting the rest of the kernel for
__GFP_REPEAT to be used here but that you mean the aoe driver's
initialization should behave like the other initialization code in the
kernel, and nobody else is using __GFP_REPEAT, so aoe should
follow suit. Please let me know if I'm misinterpreting your feedback.
Anyway, it's fine with me if you want to remove __GFP_REPEAT, or
I can resubmit this patch without it if you prefer.
--
Ed Cashin
ecas...@coraid.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
