[PATCH] ibacm: Handle EP expiration time

[PATCH] ibacm: Handle EP expiration time

[Yuval Shaia]

Old destination records are removed from EP's dest_map after timeout is
expired in order to maintain a correct cache.

Signed-off-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
 prov/acmp/src/acmp.c |   34 +++++++++++++++++++++++++---------
 1 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/prov/acmp/src/acmp.c b/prov/acmp/src/acmp.c
index ec64631..607c97f 100644
--- a/prov/acmp/src/acmp.c
+++ b/prov/acmp/src/acmp.c
@@ -356,6 +356,22 @@ acmp_put_dest(struct acmp_dest *dest)
 	}
 }
 
+/* Caller must hold ep lock. */
+static void
+acmp_remove_dest(struct acmp_ep *ep, struct acmp_dest *dest)
+{
+	acm_log(2, "%s\n", dest->name);
+	tdelete(dest->address, &ep->dest_map[dest->addr_type - 1],
+		acmp_compare_dest);
+	acmp_put_dest(dest);
+}
+
+static inline is_dest_ready(struct acmp_dest *dest)
+{
+	return dest->state == ACMP_READY &&
+	       dest->addr_timeout != 0xFFFFFFFFFFFFFFFF;
+}
+
 static struct acmp_dest *
 acmp_acquire_dest(struct acmp_ep *ep, uint8_t addr_type, const uint8_t *addr)
 {
@@ -366,6 +382,15 @@ acmp_acquire_dest(struct acmp_ep *ep, uint8_t addr_type, const uint8_t *addr)
 	acm_log(2, "%s\n", log_data);
 	lock_acquire(&ep->lock);
 	dest = acmp_get_dest(ep, addr_type, addr);
+	if (dest && is_dest_ready(dest)) {
+		acm_log(2, "Record valid for the next %ld minute(s)\n",
+			dest->addr_timeout - time_stamp_min());
+		if (time_stamp_min() >= dest->addr_timeout) {
+			acm_log(2, "Record expiered\n");
+			acmp_remove_dest(ep, dest);
+			dest = 0;
+		}
+	}
 	if (!dest) {
 		dest = acmp_alloc_dest(addr_type, addr);
 		if (dest) {
@@ -378,15 +403,6 @@ acmp_acquire_dest(struct acmp_ep *ep, uint8_t addr_type, const uint8_t *addr)
 	return dest;
 }
 
-/* Caller must hold ep lock. */
-//static void
-//acmp_remove_dest(struct acmp_ep *ep, struct acmp_dest *dest)
-//{
-//	acm_log(2, "%s\n", dest->name);
-//	tdelete(dest->address, &ep->dest_map[dest->addr_type - 1], acmp_compare_dest);
-//	acmp_put_dest(dest);
-//}
-
 static struct acmp_request *acmp_alloc_req(uint64_t id, struct acm_msg *msg)
 {
 	struct acmp_request *req;
-- 
1.7.1

--
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: [PATCH] ibacm: Handle EP expiration time

[Hefty, Sean]

> +static inline is_dest_ready(struct acmp_dest *dest)
> +{
> +	return dest->state == ACMP_READY &&
> +	       dest->addr_timeout != 0xFFFFFFFFFFFFFFFF;
> +}

I found including the timeout check here to be confusing.

> +
>  static struct acmp_dest *
>  acmp_acquire_dest(struct acmp_ep *ep, uint8_t addr_type, const uint8_t
> *addr)
>  {
> @@ -366,6 +382,15 @@ acmp_acquire_dest(struct acmp_ep *ep, uint8_t
> addr_type, const uint8_t *addr)
>  	acm_log(2, "%s\n", log_data);
>  	lock_acquire(&ep->lock);
>  	dest = acmp_get_dest(ep, addr_type, addr);
> +	if (dest && is_dest_ready(dest)) {

I think it would be clearer to just perform the state check here and either avoid the timeout check or merge it with the if statement below.

> +		acm_log(2, "Record valid for the next %ld minute(s)\n",
> +			dest->addr_timeout - time_stamp_min());
> +		if (time_stamp_min() >= dest->addr_timeout) {
> +			acm_log(2, "Record expiered\n");

Nit: misspelled expired.

> +			acmp_remove_dest(ep, dest);
> +			dest = 0;

Please use NULL in place of 0.

> +		}
> +	}
>  	if (!dest) {
>  		dest = acmp_alloc_dest(addr_type, addr);
>  		if (dest) {
> @@ -378,15 +403,6 @@ acmp_acquire_dest(struct acmp_ep *ep, uint8_t
> addr_type, const uint8_t *addr)
>  	return dest;
>  }
> 
> -/* Caller must hold ep lock. */
> -//static void
> -//acmp_remove_dest(struct acmp_ep *ep, struct acmp_dest *dest)
> -//{
> -//	acm_log(2, "%s\n", dest->name);
> -//	tdelete(dest->address, &ep->dest_map[dest->addr_type - 1],
> acmp_compare_dest);
> -//	acmp_put_dest(dest);
> -//}
> -
>  static struct acmp_request *acmp_alloc_req(uint64_t id, struct acm_msg
> *msg)
>  {
>  	struct acmp_request *req;

I'm fine with the email posting of the patch, but it's easier for me to deal with a github pull request.  If you could add a PR as well, I'd appreciate it.

- Sean
--
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: [PATCH] ibacm: Handle EP expiration time

[Yuval Shaia]

On Fri, Sep 16, 2016 at 07:38:29PM +0000, Hefty, Sean wrote:
> > +static inline is_dest_ready(struct acmp_dest *dest)
> > +{
> > +	return dest->state == ACMP_READY &&
> > +	       dest->addr_timeout != 0xFFFFFFFFFFFFFFFF;
> > +}
> 
> I found including the timeout check here to be confusing.

Checking state is not enough as there are places where add_timeout is not
set while state still is.

> 
> > +
> >  static struct acmp_dest *
> >  acmp_acquire_dest(struct acmp_ep *ep, uint8_t addr_type, const uint8_t
> > *addr)
> >  {
> > @@ -366,6 +382,15 @@ acmp_acquire_dest(struct acmp_ep *ep, uint8_t
> > addr_type, const uint8_t *addr)
> >  	acm_log(2, "%s\n", log_data);
> >  	lock_acquire(&ep->lock);
> >  	dest = acmp_get_dest(ep, addr_type, addr);
> > +	if (dest && is_dest_ready(dest)) {
> 
> I think it would be clearer to just perform the state check here and either avoid the timeout check or merge it with the if statement below.

As stated above, i cannot ignore the timeout so will merge it here.

> 
> > +		acm_log(2, "Record valid for the next %ld minute(s)\n",
> > +			dest->addr_timeout - time_stamp_min());
> > +		if (time_stamp_min() >= dest->addr_timeout) {
> > +			acm_log(2, "Record expiered\n");
> 
> Nit: misspelled expired.

Oops, thanks!

> 
> > +			acmp_remove_dest(ep, dest);
> > +			dest = 0;
> 
> Please use NULL in place of 0.

Will do.

> 
> > +		}
> > +	}
> >  	if (!dest) {
> >  		dest = acmp_alloc_dest(addr_type, addr);
> >  		if (dest) {
> > @@ -378,15 +403,6 @@ acmp_acquire_dest(struct acmp_ep *ep, uint8_t
> > addr_type, const uint8_t *addr)
> >  	return dest;
> >  }
> > 
> > -/* Caller must hold ep lock. */
> > -//static void
> > -//acmp_remove_dest(struct acmp_ep *ep, struct acmp_dest *dest)
> > -//{
> > -//	acm_log(2, "%s\n", dest->name);
> > -//	tdelete(dest->address, &ep->dest_map[dest->addr_type - 1],
> > acmp_compare_dest);
> > -//	acmp_put_dest(dest);
> > -//}
> > -
> >  static struct acmp_request *acmp_alloc_req(uint64_t id, struct acm_msg
> > *msg)
> >  {
> >  	struct acmp_request *req;
> 
> I'm fine with the email posting of the patch, but it's easier for me to deal with a github pull request.  If you could add a PR as well, I'd appreciate it.

Sure, will post v1 also as PR.

> 
> - Sean
> --
> 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
--
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: [PATCH] ibacm: Handle EP expiration time

[Hefty, Sean]

> > >  static struct acmp_dest *
> > >  acmp_acquire_dest(struct acmp_ep *ep, uint8_t addr_type, const
> uint8_t
> > > *addr)
> > >  {
> > > @@ -366,6 +382,15 @@ acmp_acquire_dest(struct acmp_ep *ep, uint8_t
> > > addr_type, const uint8_t *addr)
> > >  	acm_log(2, "%s\n", log_data);
> > >  	lock_acquire(&ep->lock);
> > >  	dest = acmp_get_dest(ep, addr_type, addr);
> > > +	if (dest && is_dest_ready(dest)) {
> >
> > I think it would be clearer to just perform the state check here and
> either avoid the timeout check or merge it with the if statement below.
> 
> As stated above, i cannot ignore the timeout so will merge it here.
> 
> >
> > > +		acm_log(2, "Record valid for the next %ld minute(s)\n",
> > > +			dest->addr_timeout - time_stamp_min());
> > > +		if (time_stamp_min() >= dest->addr_timeout) {

We also check addr_timeout here.  (Sorry, it's been a long while since I've looked at this code.)  My question regarding timeouts are around why do we need two checks?
--
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: [PATCH] ibacm: Handle EP expiration time

[Yuval Shaia]

On Mon, Sep 19, 2016 at 03:51:22PM +0000, Hefty, Sean wrote:
> > > >  static struct acmp_dest *
> > > >  acmp_acquire_dest(struct acmp_ep *ep, uint8_t addr_type, const
> > uint8_t
> > > > *addr)
> > > >  {
> > > > @@ -366,6 +382,15 @@ acmp_acquire_dest(struct acmp_ep *ep, uint8_t
> > > > addr_type, const uint8_t *addr)
> > > >  	acm_log(2, "%s\n", log_data);
> > > >  	lock_acquire(&ep->lock);
> > > >  	dest = acmp_get_dest(ep, addr_type, addr);
> > > > +	if (dest && is_dest_ready(dest)) {
> > >
> > > I think it would be clearer to just perform the state check here and
> > either avoid the timeout check or merge it with the if statement below.
> > 
> > As stated above, i cannot ignore the timeout so will merge it here.
> > 
> > >
> > > > +		acm_log(2, "Record valid for the next %ld minute(s)\n",
> > > > +			dest->addr_timeout - time_stamp_min());
> > > > +		if (time_stamp_min() >= dest->addr_timeout) {
> 
> We also check addr_timeout here.  (Sorry, it's been a long while since I've looked at this code.)  My question regarding timeouts are around why do we need two checks?

First one is for log only.
If you think it is not needed then i'll remove it, otherwise i will take it
out to an helper variable and use it in both.

> --
> 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
--
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: [PATCH] ibacm: Handle EP expiration time

[Hefty, Sean]

> > > > >  static struct acmp_dest *
> > > > >  acmp_acquire_dest(struct acmp_ep *ep, uint8_t addr_type, const
> > > uint8_t
> > > > > *addr)
> > > > >  {
> > > > > @@ -366,6 +382,15 @@ acmp_acquire_dest(struct acmp_ep *ep,
> uint8_t
> > > > > addr_type, const uint8_t *addr)
> > > > >  	acm_log(2, "%s\n", log_data);
> > > > >  	lock_acquire(&ep->lock);
> > > > >  	dest = acmp_get_dest(ep, addr_type, addr);
> > > > > +	if (dest && is_dest_ready(dest)) {
> > > >
> > > > I think it would be clearer to just perform the state check here
> and
> > > either avoid the timeout check or merge it with the if statement
> below.
> > >
> > > As stated above, i cannot ignore the timeout so will merge it here.
> > >
> > > >
> > > > > +		acm_log(2, "Record valid for the next %ld
> minute(s)\n",
> > > > > +			dest->addr_timeout - time_stamp_min());
> > > > > +		if (time_stamp_min() >= dest->addr_timeout) {
> >
> > We also check addr_timeout here.  (Sorry, it's been a long while
> since I've looked at this code.)  My question regarding timeouts are
> around why do we need two checks?
> 
> First one is for log only.

okay

> If you think it is not needed then i'll remove it, otherwise i will
> take it
> out to an helper variable and use it in both.

I think we're good then.
--
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