RE: [linux-rdma/rdma-core] libibverb/examples: Add command line option to enable buffer validation (#273)

RE: [linux-rdma/rdma-core] libibverb/examples: Add command line option to enable buffer validation (#273)

[Amrani, Ram]

[resend as plain text]

Hi Jason, Yuval,
I’ve started a review on this a couple of days ago. It’s the first time I’ve done this.
Did you get e-mails about it? How come it gets merged without response?
Please comment on my remarks.

Thanks,
Ram


From: Jason Gunthorpe [mailto:notifications@github.com] 
Sent: Wednesday, December 13, 2017 11:34 PM
To: linux-rdma/rdma-core <mailto:rdma-core@noreply.github.com>
Cc: ramranix <mailto:ram.amrani@gmail.com>; Comment <mailto:comment@noreply.github.com>
Subject: Re: [linux-rdma/rdma-core] libibverb/examples: Add command line option to enable buffer validation (#273)

Merged https://github.com/linux-rdma/rdma-core/pull/273.
—
You are receiving this because you commented.
Reply to this email directly, https://github.com/linux-rdma/rdma-core/pull/273#event-1386542099, or https://github.com/notifications/unsubscribe-auth/ANMBVianRscywuH7irUpKgtlyDgL-UXEks5tAELZgaJpZM4Q99Mf.

RE: [linux-rdma/rdma-core] libibverb/examples: Add command line option to enable buffer validation (#273)

[Amrani, Ram]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1021 bytes --]

> Hi Jason, Yuval,
> I’ve started a review on this a couple of days ago. It’s the first time I’ve done this.
> Did you get e-mails about it? How come it gets merged without response?
> Please comment on my remarks.
> 
> Thanks,
> Ram
> 

I still didn't see any e-mail.
Probably the review doesn't work after the patch has been committed.
My comments:

+	printf("  -c, --chk	            validate received buffer\n");
Why not have the default set to 'validate'?
The only reason not to use validate is for performance and I don't think this application checks that...
Still, if this is important then the user can choose to disable validation.

+			for (int i = 0; i < size; i += page_size)
I expect (and IMHO users expect too) that data validation is on the entire buffer. What is happening here is that only a single byte per page is checked.

Thanks,
Ram


N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš

Re: [linux-rdma/rdma-core] libibverb/examples: Add command line option to enable buffer validation (#273)

[Yuval Shaia]

On Tue, Dec 19, 2017 at 09:14:05AM +0000, Amrani, Ram wrote:
> > Hi Jason, Yuval,
> > I’ve started a review on this a couple of days ago. It’s the first time I’ve done this.
> > Did you get e-mails about it? How come it gets merged without response?
> > Please comment on my remarks.
> > 
> > Thanks,
> > Ram
> > 
> 
> I still didn't see any e-mail.
> Probably the review doesn't work after the patch has been committed.

Have no idea why?
So, for rdma-core, i understood that PR on github is much more powerful
than linux-rdma mailing list but i don't mind running on both next time.

> My comments:
> 
> +	printf("  -c, --chk	            validate received buffer\n");
> Why not have the default set to 'validate'?

Because we don't want to break existing user-created scripts so default
should be - "the same as it was!".

> The only reason not to use validate is for performance and I don't think this application checks that...

Yeah, the validation is out of the scope of performance accounting.

> Still, if this is important then the user can choose to disable validation.
> 
> +			for (int i = 0; i < size; i += page_size)
> I expect (and IMHO users expect too) that data validation is on the entire buffer. What is happening here is that only a single byte per page is checked.

Because i believe (and sure i maybe mistaken here) is that errors may happen
in a page boundaries. If a full buffer validation is needed then we can add
a check validation level such as "-c 0" for page boundaries and "-c 1" for
full check.

> 
> Thanks,
> Ram
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [linux-rdma/rdma-core] libibverb/examples: Add command line option to enable buffer validation (#273)

[Amrani, Ram]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 427 bytes --]

> Because i believe (and sure i maybe mistaken here) is that errors may happen
> in a page boundaries. If a full buffer validation is needed then we can add
> a check validation level such as "-c 0" for page boundaries and "-c 1" for
> full check.

That would be great.

Thanks!
Ram

N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš

Re: [linux-rdma/rdma-core] libibverb/examples: Add command line option to enable buffer validation (#273)

[Jason Gunthorpe]

On Wed, Dec 20, 2017 at 08:22:21PM +0200, Yuval Shaia wrote:
> On Tue, Dec 19, 2017 at 09:14:05AM +0000, Amrani, Ram wrote:
> > > Hi Jason, Yuval,
> > > I’ve started a review on this a couple of days ago. It’s the first time I’ve done this.
> > > Did you get e-mails about it? How come it gets merged without response?
> > > Please comment on my remarks.
> > > 
> > > Thanks,
> > > Ram
> > > 
> > 
> > I still didn't see any e-mail.
> > Probably the review doesn't work after the patch has been committed.
> 
> Have no idea why?
> So, for rdma-core, i understood that PR on github is much more powerful
> than linux-rdma mailing list but i don't mind running on both next time.

PR emails only go to a few people. I saw an email notice from Ram after he
posted his review.. Perhaps the submitter is not included after the PR
is closed? Maybe your github notifications are not working?

This limited audiance is a big part of why Leon asks for patches to be
sent to the mailing list as well as in a PR..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html