Process question: reviewing older patches

Process question: reviewing older patches

[Jay Rolette]

There's a fairly old KNI patch (http://dpdk.org/dev/patchwork/patch/84/)
that I reviewed, but I'm not seeing how to submit my "Reviewed-by" when I
don't have any of the emails from the patch in my mail client.

I can copy the text from the 'mbox' link in Patchwork into an email, but
I'm guessing that may not make the patch toolchain happy.

What's the right way to do this?

Thanks,
Jay

Re: Process question: reviewing older patches

[Thomas Monjalon]

2015-01-28 09:52, Jay Rolette:
> There's a fairly old KNI patch (http://dpdk.org/dev/patchwork/patch/84/)
> that I reviewed, but I'm not seeing how to submit my "Reviewed-by" when I
> don't have any of the emails from the patch in my mail client.
> 
> I can copy the text from the 'mbox' link in Patchwork into an email, but
> I'm guessing that may not make the patch toolchain happy.
> 
> What's the right way to do this?

I think you should try to open the mbox file with your mail client and reply.
In my case, I had to rename it into .mbox (was .patch).

Thanks for reviewing
-- 
Thomas

Re: Process question: reviewing older patches

[Neil Horman]

On Wed, Jan 28, 2015 at 09:52:48AM -0600, Jay Rolette wrote:
> There's a fairly old KNI patch (http://dpdk.org/dev/patchwork/patch/84/)
> that I reviewed, but I'm not seeing how to submit my "Reviewed-by" when I
> don't have any of the emails from the patch in my mail client.
> 
> I can copy the text from the 'mbox' link in Patchwork into an email, but
> I'm guessing that may not make the patch toolchain happy.
> 
> What's the right way to do this?
> 
Just grab the message id from the patchwork site, and list it in the envelope
headers in-reply-to: field when you respond.  You won't have the rest of the
conversation field in the thread, but you will respond properly to the thread,
and patchwork will pick up the ACK
Neil

> Thanks,
> Jay
> 

Re: Process question: reviewing older patches

[Jay Rolette]

Thanks Thomas and Neil. Sadly, no joy. While I generally like gmail for my
mail, there's not a reasonable way to import the mbox file or to control
the message id.

If someone else wants to resend the message to the list, I can reply to
that. Otherwise, here are the relevant bits from the original patch email:

>From patchwork Wed Jul 23 06:45:12 2014
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: [dpdk-dev] kni: optimizing the rte_kni_rx_burst
From: Hemant Agrawal <Hemant-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
X-Patchwork-Id: 84
Message-Id: <14060979121185-git-send-email-Hemant-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
To: <dev-VfR2kkLFssw@public.gmane.org>
Date: Wed, 23 Jul 2014 12:15:12 +0530

The current implementation of rte_kni_rx_burst polls the fifo for buffers.
Irrespective of success or failure, it allocates the mbuf and try to put
them into the alloc_q
if the buffers are not added to alloc_q, it frees them.
This waste lots of cpu cycles in allocating and freeing the buffers if
alloc_q is full.

The logic has been changed to:
1. Initially allocand add buffer(burstsize) to alloc_q
2. Add buffers to alloc_q only when you are pulling out the buffers.

Signed-off-by: Hemant Agrawal <Hemant-KZfg59tc24xl57MIdRCFDg@public.gmane.org>

---
lib/librte_kni/rte_kni.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
index 76feef4..01e85f8 100644
--- a/lib/librte_kni/rte_kni.c
+++ b/lib/librte_kni/rte_kni.c
@@ -263,6 +263,9 @@ rte_kni_alloc(struct rte_mempool *pktmbuf_pool,

  ctx->in_use = 1;

+ /* Allocate mbufs and then put them into alloc_q */
+ kni_allocate_mbufs(ctx);
+
  return ctx;

 fail:
@@ -369,8 +372,9 @@ rte_kni_rx_burst(struct rte_kni *kni, struct rte_mbuf
**mbufs, unsigned num)
 {
  unsigned ret = kni_fifo_get(kni->tx_q, (void **)mbufs, num);

- /* Allocate mbufs and then put them into alloc_q */
- kni_allocate_mbufs(kni);
+ /* If buffers removed, allocate mbufs and then put them into alloc_q */
+ if(ret)
+ kni_allocate_mbufs(kni);

  return ret;
 }

The patch looks good from a DPDK 1.6r2 viewpoint. We saw the same behavior
in our app and ended up avoiding it higher in the stack (in our code).

Reviewed-by: Jay Rolette <rolette-bIuJOMs36aleGPcbtGPokg@public.gmane.org>

Jay

On Wed, Jan 28, 2015 at 10:49 AM, Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> wrote:

> On Wed, Jan 28, 2015 at 09:52:48AM -0600, Jay Rolette wrote:
> > There's a fairly old KNI patch (http://dpdk.org/dev/patchwork/patch/84/)
> > that I reviewed, but I'm not seeing how to submit my "Reviewed-by" when I
> > don't have any of the emails from the patch in my mail client.
> >
> > I can copy the text from the 'mbox' link in Patchwork into an email, but
> > I'm guessing that may not make the patch toolchain happy.
> >
> > What's the right way to do this?
> >
> Just grab the message id from the patchwork site, and list it in the
> envelope
> headers in-reply-to: field when you respond.  You won't have the rest of
> the
> conversation field in the thread, but you will respond properly to the
> thread,
> and patchwork will pick up the ACK
> Neil
>
> > Thanks,
> > Jay
> >
>

Re: Process question: reviewing older patches

[Neil Horman]

On Wed, Jan 28, 2015 at 02:57:58PM -0600, Jay Rolette wrote:
> Thanks Thomas and Neil. Sadly, no joy. While I generally like gmail for my
> mail, there's not a reasonable way to import the mbox file or to control
> the message id.
> 
Sure there is, you just need to select an appropriate MUA.  You can't use the
web interface for this.  Enable imap access to your gmail account, and setup an
MUA like mutt to point to it.  Then the mutt client can open the mbox file, or
you can fill out the in-reply-to: header manually.

Neil

Re: Process question: reviewing older patches

[Jay Rolette]

On Wed, Jan 28, 2015 at 3:23 PM, Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> wrote:

> On Wed, Jan 28, 2015 at 02:57:58PM -0600, Jay Rolette wrote:
> > Thanks Thomas and Neil. Sadly, no joy. While I generally like gmail for
> my
> > mail, there's not a reasonable way to import the mbox file or to control
> > the message id.
> >
> Sure there is, you just need to select an appropriate MUA.  You can't use
> the
> web interface for this.  Enable imap access to your gmail account, and
> setup an
> MUA like mutt to point to it.  Then the mutt client can open the mbox
> file, or
> you can fill out the in-reply-to: header manually.
>

True. I meant there's not a way to do this in the mail interface I use.
Honestly, while I was happy to do the review since I've been digging into
KNI anyway, I'm not going to spend time setting up an MUA just for this.

Jay

Re: Process question: reviewing older patches

[Neil Horman]

On Thu, Jan 29, 2015 at 07:38:30AM -0600, Jay Rolette wrote:
> On Wed, Jan 28, 2015 at 3:23 PM, Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> wrote:
> 
> > On Wed, Jan 28, 2015 at 02:57:58PM -0600, Jay Rolette wrote:
> > > Thanks Thomas and Neil. Sadly, no joy. While I generally like gmail for
> > my
> > > mail, there's not a reasonable way to import the mbox file or to control
> > > the message id.
> > >
> > Sure there is, you just need to select an appropriate MUA.  You can't use
> > the
> > web interface for this.  Enable imap access to your gmail account, and
> > setup an
> > MUA like mutt to point to it.  Then the mutt client can open the mbox
> > file, or
> > you can fill out the in-reply-to: header manually.
> >
> 
> True. I meant there's not a way to do this in the mail interface I use.
> Honestly, while I was happy to do the review since I've been digging into
> KNI anyway, I'm not going to spend time setting up an MUA just for this.
> 
> Jay

well, ok.  But if you ever want to be able to respond to an email you've deleted,
you need an MUA that can import mbox files from an archive, or at least allow
editing of header fields.

FWIW, its not hard to do, heres a redacted version of my gmail .muttrc file:

set folder=imaps://<my email address>@imap.gmail.com
set spoolfile=imaps://<my email address>@imap.gmail.com/INBOX
set ssl_starttls=yes
set editor=vi
set sort=threads
set smtp_url=smtps://<my email address>@smtp.gmail.com
set from="Neil Horman <my email address>"