lkml.org 
[lkml]   [2012]   [Dec]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 1/7] aoe: improve handling of misbehaving network paths
On Dec 4, 2012, at 6:39 PM, Andrew Morton wrote:

> On Mon, 3 Dec 2012 20:40:55 -0500
> Ed Cashin <ecashin@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
ecashin@coraid.com




\
 
 \ /
  Last update: 2012-12-06 17:42    [W:0.062 / U:1.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site