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

[daniel]

daniel 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 4:

(8 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
Done


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.
tlli is a mandatory field for SUSPEND/RESUME and in line 860 osmo_tlv_prot_parse would fail if any mandatory field isn't present.

Other branches skip these kind of checks as well.


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
Done


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


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
Done


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  […]
Yeah, I thought about it as an optimization. I'll add a TODO for now


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
> /* */
Done


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. […]
We don't really need sub-second resolution anyway (and I changed the tlli_cache.timeout from timespec after a comment from Harald)

https://gerrit.osmocom.org/c/osmo-sgsn/+/21960/1/include/osmocom/sgsn/gb_proxy.h#177

I agree that comparing with timespec would probably be nicer here.



-- 
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: 4
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: Mon, 11 Jan 2021 04:04:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210111/273f7284/attachment.htm>