Change in osmo-sgsn[master]: gbproxy: Implement TLLI cache and use it for SUSPEND/RESUME

[pespin]

pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/21960 )

Change subject: gbproxy: Implement TLLI cache and use it for SUSPEND/RESUME
......................................................................


Patch Set 3: Code-Review-1

(9 comments)

https://gerrit.osmocom.org/c/osmo-sgsn/+/21960/3/include/osmocom/sgsn/gb_proxy.h 
File include/osmocom/sgsn/gb_proxy.h:

https://gerrit.osmocom.org/c/osmo-sgsn/+/21960/3/include/osmocom/sgsn/gb_proxy.h@175 
PS3, Line 175: struct gbproxy_tlli_cache {
cache_entry or cache_item would be more descriptive imho


https://gerrit.osmocom.org/c/osmo-sgsn/+/21960/3/src/gbproxy/gb_proxy.c 
File src/gbproxy/gb_proxy.c:

https://gerrit.osmocom.org/c/osmo-sgsn/+/21960/3/src/gbproxy/gb_proxy.c@896 
PS3, Line 896: 		tlli = osmo_load32be(TLVP_VAL(&tp, BSSGP_IE_TLLI));
would be great checking if TLLI is really there instead of directly de-referencing it.


https://gerrit.osmocom.org/c/osmo-sgsn/+/21960/3/src/gbproxy/gb_proxy.c@1164 
PS3, Line 1164: 		uint32_t tlli = osmo_load32be(TLVP_VAL(&tp, BSSGP_IE_TLLI));
same


https://gerrit.osmocom.org/c/osmo-sgsn/+/21960/3/src/gbproxy/gb_proxy.c@1170 
PS3, Line 1170: 			// FIXME
FIX what?


https://gerrit.osmocom.org/c/osmo-sgsn/+/21960/3/src/gbproxy/gb_proxy.c@1400 
PS3, Line 1400: void tlli_cache_cleanup(void *data)
static


https://gerrit.osmocom.org/c/osmo-sgsn/+/21960/3/src/gbproxy/gb_proxy.c@1405 
PS3, Line 1405: 	osmo_timer_schedule(&cfg->tlli_cache.timer, 2, 0);
Probably worth avoid reescheduling it if the tlli cache is empty, and reenabling it when first item is added.


https://gerrit.osmocom.org/c/osmo-sgsn/+/21960/3/src/gbproxy/gb_proxy_peer.c 
File src/gbproxy/gb_proxy_peer.c:

https://gerrit.osmocom.org/c/osmo-sgsn/+/21960/3/src/gbproxy/gb_proxy_peer.c@270 
PS3, Line 270: 		// Update the entry if it already exists
/* */


https://gerrit.osmocom.org/c/osmo-sgsn/+/21960/3/src/gbproxy/gb_proxy_peer.c@320 
PS3, Line 320: 	expiry = now.tv_sec - cfg->tlli_cache.timeout;
I'd go or using timespec completely.
include <osmocom/core/timer_compat.h> and you have there all the timespeccmp, timespecsub, etc. available.


https://gerrit.osmocom.org/c/osmo-sgsn/+/21960/3/src/gbproxy/gb_proxy_peer.c@323 
PS3, Line 323: 		if (tlli_cache->tstamp.tv_sec < expiry) {
timespeccmp



-- 
To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/21960
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-sgsn
Gerrit-Branch: master
Gerrit-Change-Id: I42adf70f560d2bb358a9e1c7614281e8d2967568
Gerrit-Change-Number: 21960
Gerrit-PatchSet: 3
Gerrit-Owner: daniel <dwillmann at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-CC: laforge <laforge at osmocom.org>
Gerrit-Comment-Date: Fri, 08 Jan 2021 13:26:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210108/55e0398d/attachment.htm>