| Message ID | 1459385402-28449-1-git-send-email-shamir.rabinovitch@oracle.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello. On 3/31/2016 3:50 AM, shamir rabinovitch wrote: > Issue can be seen on platforms that use 8K and above page size > while rds fragment size is 4K. On those platforms single page is > shared between 2 or more rds fragments. Each fragment has it's own Its. > offeset and rds cong map code need to take this offset to account. Offset. What is "cong", congestion? > Not taking this offset to account lead to reading the data fragment > as congestion map fragment and hang of the rds transmit due to far > cong map corruption. > > Reviewed-by: Wengang Wang <wen.gang.wang@oracle.com> > Reviewed-by: Ajaykumar Hotchandani <ajaykumar.hotchandani@oracle.com> > Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com> > Tested-by: Anand Bibhuti <anand.bibhuti@oracle.com> These should be after your sign-off, not before. > Signed-off-by: shamir rabinovitch <shamir.rabinovitch@oracle.com> [...] MBR, Sergei
On Thu, Mar 31, 2016 at 04:13:35PM +0300, Sergei Shtylyov wrote: > Hello. > > On 3/31/2016 3:50 AM, shamir rabinovitch wrote: > > >Issue can be seen on platforms that use 8K and above page size > >while rds fragment size is 4K. On those platforms single page is > >shared between 2 or more rds fragments. Each fragment has it's own > > Its. Fixed in next version. Thanks. > > >offeset and rds cong map code need to take this offset to account. > > Offset. What is "cong", congestion? 'Offset' is in middle of the sentence so it is OK as-is. Cong is short hand of congestion. It will be replaces with the full word in next version. > > >Not taking this offset to account lead to reading the data fragment > >as congestion map fragment and hang of the rds transmit due to far > >cong map corruption. > > > >Reviewed-by: Wengang Wang <wen.gang.wang@oracle.com> > >Reviewed-by: Ajaykumar Hotchandani <ajaykumar.hotchandani@oracle.com> > >Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com> > >Tested-by: Anand Bibhuti <anand.bibhuti@oracle.com> > > These should be after your sign-off, not before. Thanks for the comment. Will be fixed in next version. > > >Signed-off-by: shamir rabinovitch <shamir.rabinovitch@oracle.com> > [...] > > MBR, Sergei > Thanks for the review. BR, Shamir
On 3/31/2016 5:20 PM, Shamir Rabinovitch wrote: >>> Issue can be seen on platforms that use 8K and above page size >>> while rds fragment size is 4K. On those platforms single page is >>> shared between 2 or more rds fragments. Each fragment has it's own >> >> Its. > > Fixed in next version. > Thanks. > >> >>> offeset and rds cong map code need to take this offset to account. >> >> Offset. What is "cong", congestion? > > 'Offset' is in middle of the sentence so it is OK as-is. No, "offeset" is not OK. :-) [...] >>> Signed-off-by: shamir rabinovitch <shamir.rabinovitch@oracle.com> [...] MBR, Sergei
On Thu, Mar 31, 2016 at 05:23:30PM +0300, Sergei Shtylyov wrote: > On 3/31/2016 5:20 PM, Shamir Rabinovitch wrote: > > >>>Issue can be seen on platforms that use 8K and above page size > >>>while rds fragment size is 4K. On those platforms single page is > >>>shared between 2 or more rds fragments. Each fragment has it's own > >> > >> Its. > > > >Fixed in next version. > >Thanks. > > > >> > >>>offeset and rds cong map code need to take this offset to account. > >> > >> Offset. What is "cong", congestion? > > > >'Offset' is in middle of the sentence so it is OK as-is. > > No, "offeset" is not OK. :-) Oh. Typo. Sorry. Will fix! :-) > > [...] > > >>>Signed-off-by: shamir rabinovitch <shamir.rabinovitch@oracle.com> > [...] > > MBR, Sergei >
Hi Shamir, Nice to see this one soon on the list, Just to make $subject more relevant. How about below? RDS: fix congestion map corruption for PAGE_SIZE > 8k On 3/30/16 5:50 PM, shamir rabinovitch wrote: > Issue can be seen on platforms that use 8K and above page size > while rds fragment size is 4K. On those platforms single page is > shared between 2 or more rds fragments. Each fragment has it's own > offeset and rds cong map code need to take this offset to account. > Not taking this offset to account lead to reading the data fragment > as congestion map fragment and hang of the rds transmit due to far > cong map corruption. > > Reviewed-by: Wengang Wang <wen.gang.wang@oracle.com> > Reviewed-by: Ajaykumar Hotchandani <ajaykumar.hotchandani@oracle.com> > Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com> > Tested-by: Anand Bibhuti <anand.bibhuti@oracle.com> > > Signed-off-by: shamir rabinovitch <shamir.rabinovitch@oracle.com> > --- > net/rds/ib_recv.c | 2 +- > net/rds/iw_recv.c | 2 +- > net/rds/page.c | 5 +++-- > 3 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c > index 977fb86..abc8cc8 100644 > --- a/net/rds/ib_recv.c > +++ b/net/rds/ib_recv.c > @@ -796,7 +796,7 @@ static void rds_ib_cong_recv(struct rds_connection *conn, > > addr = kmap_atomic(sg_page(&frag->f_sg)); > > - src = addr + frag_off; > + src = addr + frag->f_sg.offset + frag_off; > dst = (void *)map->m_page_addrs[map_page] + map_off; > for (k = 0; k < to_copy; k += 8) { > /* Record ports that became uncongested, ie > diff --git a/net/rds/iw_recv.c b/net/rds/iw_recv.c If you refresh the patch against 4.6-rc1, you won't need to patch iw_recv.c :-) > diff --git a/net/rds/page.c b/net/rds/page.c > index 5a14e6d..715cbaa 100644 > --- a/net/rds/page.c > +++ b/net/rds/page.c > @@ -135,8 +135,9 @@ int rds_page_remainder_alloc(struct scatterlist *scat, unsigned long bytes, > if (rem->r_offset != 0) > rds_stats_inc(s_page_remainder_hit); > > - rem->r_offset += bytes; > - if (rem->r_offset == PAGE_SIZE) { > + /* some hw (e.g. sparc) require aligned memory */ > + rem->r_offset += ALIGN(bytes, 8); > + if (rem->r_offset >= PAGE_SIZE) { > __free_page(rem->r_page); > rem->r_page = NULL; > } > This hunk I missed out looks like. This doesn't belong to the $subject patch. Could you please add this in separate patch. I will need more than just "some hw (e.g. sparc) require aligned memory" Once you fix these, please repost the updated version, and I will add them to the 4.7 queue. Thanks !! Regards, Santosh
diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c index 977fb86..abc8cc8 100644 --- a/net/rds/ib_recv.c +++ b/net/rds/ib_recv.c @@ -796,7 +796,7 @@ static void rds_ib_cong_recv(struct rds_connection *conn, addr = kmap_atomic(sg_page(&frag->f_sg)); - src = addr + frag_off; + src = addr + frag->f_sg.offset + frag_off; dst = (void *)map->m_page_addrs[map_page] + map_off; for (k = 0; k < to_copy; k += 8) { /* Record ports that became uncongested, ie diff --git a/net/rds/iw_recv.c b/net/rds/iw_recv.c index a66d179..62a1738 100644 --- a/net/rds/iw_recv.c +++ b/net/rds/iw_recv.c @@ -585,7 +585,7 @@ static void rds_iw_cong_recv(struct rds_connection *conn, addr = kmap_atomic(frag->f_page); - src = addr + frag_off; + src = addr + frag->f_offset + frag_off; dst = (void *)map->m_page_addrs[map_page] + map_off; for (k = 0; k < to_copy; k += 8) { /* Record ports that became uncongested, ie diff --git a/net/rds/page.c b/net/rds/page.c index 5a14e6d..715cbaa 100644 --- a/net/rds/page.c +++ b/net/rds/page.c @@ -135,8 +135,9 @@ int rds_page_remainder_alloc(struct scatterlist *scat, unsigned long bytes, if (rem->r_offset != 0) rds_stats_inc(s_page_remainder_hit); - rem->r_offset += bytes; - if (rem->r_offset == PAGE_SIZE) { + /* some hw (e.g. sparc) require aligned memory */ + rem->r_offset += ALIGN(bytes, 8); + if (rem->r_offset >= PAGE_SIZE) { __free_page(rem->r_page); rem->r_page = NULL; }